Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: move /etc/ to /usr/etc/ #434

Closed
wants to merge 6 commits into from
Closed

fix: move /etc/ to /usr/etc/ #434

wants to merge 6 commits into from

Conversation

bobslept
Copy link
Contributor

Fixes #428

This moves all the /etc/ directories to their corresponding /usr/etc directory. We still need copy back a few files to make the builds work. Like systemd service files or repo files.

I hope this is the correct approach.

@bobslept bobslept requested a review from castrojo as a code owner August 22, 2023 13:16
@bobslept bobslept enabled auto-merge August 22, 2023 13:16
@castrojo
Copy link
Member

This looks good to me but considering the change let's get a few more eyeballs on this. Thanks!

@bketelsen
Copy link
Member

Looks good to me too. @castrojo I'm good with merging

castrojo
castrojo previously approved these changes Aug 23, 2023
@bobslept bobslept added this pull request to the merge queue Aug 23, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 23, 2023
@bketelsen
Copy link
Member

these build errors are from the mutter-vrr package, unrelated to this PR

@castrojo castrojo added this pull request to the merge queue Aug 23, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 23, 2023
Containerfile Outdated
Comment on lines 14 to 15
COPY usr/etc/systemd /etc/systemd
COPY usr/etc/yum.repos.d /etc/yum.repos.d
Copy link
Member

@p5 p5 Aug 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth keeping both the /etc/ and /usr/ directories in the repo for easier management and understanding?

We're chucking files into /usr/etc in the repo but that's not where they end up in the image. It could be confusing when debugging issues or referencing this repo for custom images.

Or if the use-case is to store a local readonly copy of these configurations, simply:

COPY usr /usr
COPY etc /etc
COPY etc /usr/etc

Copy link
Contributor Author

@bobslept bobslept Aug 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the idea of having them in /usr/etc is that we can reset for a fresh start. So they have to be there for that operation. When you have them in /etc/ in the repo, chances may be that they don't end up in /usr/etc/.

They are available from that location on boot, but while building the container they are not. That is why I copy them in there explicit.

I have to let you all decide what is best, because I am not comfortable enough to make a good descision on this point. But I appreciate all the feedback. Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jorge and others are happy with the way you've proposed, so I'm ok with it.

I just wasn't too sure of how this would be used.

Copy link
Contributor Author

@bobslept bobslept Aug 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this ostree deployment is where the idea also came from. In cominbation with the config reset to get a fresh start.

Besides the exceptions of /var and /etc then, the rest of the contents of the tree are checked out as hard links into the repository. It’s strongly recommended that operating systems ship all of their content in /usr, but this is not a hard requirement.

But let's see what the others have to say about. Maybe you have a very good point.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the build is busted anyway, would it help if we document how it should be used? I'm thinking something like "we'll ship the configs in /usr/etc so you have a copy, here's how you override a few of them, like your distrobox.ini, etc?"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this ostree deployment is where the idea also came from. In cominbation with the config reset to get a fresh start.

I think the more relevant part of the link is: https://ostreedev.github.io/ostree/deployment/#contents-of-a-deployment

The deployment should not have a traditional UNIX /etc; instead, it should include /usr/etc. This is the “default configuration”. When OSTree creates a deployment, it performs a 3-way merge using the old default configuration, the active system’s /etc, and the new default configuration. In the final filesystem tree for a deployment then, /etc is a regular writable directory.

TLDR: The idea of this PR seems good, but after moving files (in repo) from etc to usr/etc, I think it's contrary to the goal to copy any files back to /etc/* during the build.

Specifically I'm referring to lines like:

COPY usr/etc/systemd /etc/systemd
COPY dx/usr/etc/yum.repos.d /etc/yum.repos.d
COPY framework/usr/etc/systemd /etc/systemd

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So maybe after all the best way for now is just leave the repo structure in place and just add a COPY /etc /usr/etc instead like @p5 suggested? This way we don't have to think which file goes where when doing both /etc and /usr/etc at repo level.

Or maybe even ignore this PR all together because it seems that the files we never put in /usr/etc/ at this moment are all there to have the original version of the file. Which was our first reason for doing this if i'm correct.

@castrojo
Copy link
Member

castrojo commented Aug 24, 2023

I'll work on documenting this, holding this until then (or someone else beats me to it).

@castrojo castrojo marked this pull request as draft August 24, 2023 14:39
@bobslept
Copy link
Contributor Author

I've just noticed that even without this pr there is already a copy of the file in /usr/etc/. I had edited my /etc/distrobox/distrobox.ini so it was not overwritten by a new update I did. But the new version of the file was already in /usr/etc/distrobox/distrobox.ini so I could copy that one in to restore the original one.

@castrojo
Copy link
Member

ublue-os/website#404

@castrojo
Copy link
Member

OK what am I doing here? Do we have concensus? 😄 I've PR'ed the docs and that's merged.

@bsherman
Copy link
Contributor

OK what am I doing here? Do we have concensus? 😄 I've PR'ed the docs and that's merged.

I can make some specific requests to change the PR in a way which is compatible with what has been discussed and you put on the website.

Containerfile Outdated
COPY usr /usr
COPY usr/etc/systemd /etc/systemd
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove these 2 lines:

COPY usr/etc/systemd /etc/systemd
COPY usr/etc/yum.repos.d /etc/yum.repos.d

Containerfile Outdated
COPY dx/usr /usr
COPY dx/usr/etc/yum.repos.d /etc/yum.repos.d
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this line:

COPY dx/usr/etc/yum.repos.d /etc/yum.repos.d

Containerfile Outdated
COPY framework/usr /usr
COPY framework/usr/etc/systemd /etc/systemd
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this line:

COPY framework/usr/etc/systemd /etc/systemd

Copy link
Contributor

@bsherman bsherman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have 3 comments which are requested changes which should bring things in line with what I believe was discussed and which was published to the website.

Also, this needs to be updated from main branch.

Then I think should be good to go.

@bobslept
Copy link
Contributor Author

bobslept commented Aug 27, 2023

Oh boy I think officially messed everything up now. So sorry 😢

I did run a build on my branch and getting build errors after removing those line. That was why the repo file was copied back in.

error: Packages not found: tailscale

@bsherman
Copy link
Contributor

Oh boy I think officially messed everything up now. So sorry 😢

No worries! With git and pull requests, there is rarely an unrecoverable issue.

I have a suggestion for the current situation though. Since your PR included moving a lot of files, it's harder to merge in changes made to those files.

I suggest making a fresh new fork of bluefin repo. Immediately make the needed changes and create the PR, but also link to this one in the comment so all the lovely history is connected. :-)

I'll try to approve quickly assuming it's what I expect from the last set of comments here.

@bobslept
Copy link
Contributor Author

I've created a fresh one. Lets continue this PR in #441

@bobslept bobslept closed this Aug 27, 2023
@bobslept bobslept deleted the fix-location-configs branch August 28, 2023 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move most customizations to /usr/etc
6 participants