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

cmd-buildextend-hashlist-experimental: make all directories in tmp checkout readable #3386

Merged
merged 1 commit into from Mar 6, 2023

Conversation

aaradhak
Copy link
Member

@aaradhak aaradhak commented Mar 2, 2023

With reference to the latest changes in systemd-253, on executing cmd-builextend-hashlist-experimental the default tmp dir files configuration automatically creates /etc/credstore with secure permissions. We are changing the find invocation permissions to access the /etc/credstore directory. find can't recurse into /etc/credstore, we add x so that it can recurse into it. With +, find accumulates paths and runs chmod once (or batched by the arg limit). But for this, we need it to chmod the dir before it tries to recurse into it, so we change to ;

Fixes : coreos/fedora-coreos-tracker#1429

@openshift-ci
Copy link

openshift-ci bot commented Mar 2, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@dustymabe
Copy link
Member

The commit message here needs way more context.

What is the problem?
How does this change solve the problem?
Is this fixing any opened issues?

@aaradhak
Copy link
Member Author

aaradhak commented Mar 3, 2023

The commit message here needs way more context.

What is the problem? How does this change solve the problem? Is this fixing any opened issues?

Yes, I just added it . I wanted to test it and then add the details before opening it for review.

@aaradhak
Copy link
Member Author

aaradhak commented Mar 3, 2023

@jlebon @dustymabe
Tested cmd-buildextend-hashlist-experimental locally with the changes, it is working:

[coreos-assembler]$ sudo vi /usr/lib/coreos-assembler/cmd-buildextend-hashlist-experimental 
[coreos-assembler]$ cosa buildextend-hashlist-experimental
Hash list created at builds/38.20230303.dev.0/x86_64/exp-hash.json

@aaradhak aaradhak requested a review from dustymabe March 3, 2023 18:37
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

I think we can use Closes: for the tracker issue?

Also, instead of "permission changes", how about e.g. "make all directories in tmp checkout readable"?

src/cmd-buildextend-hashlist-experimental Outdated Show resolved Hide resolved
@aaradhak aaradhak changed the title cmd-buildextend-hashlist-experimental: permission changes cmd-buildextend-hashlist-experimental: make all directories in tmp checkout readable Mar 3, 2023
@aaradhak aaradhak force-pushed the hash branch 3 times, most recently from f62dbe0 to 2ef2843 Compare March 3, 2023 20:48
@aaradhak aaradhak requested a review from jlebon March 3, 2023 20:57
@dustymabe
Copy link
Member

you need to squash the commits into one commit

@aaradhak
Copy link
Member Author

aaradhak commented Mar 3, 2023

you need to squash the commits into one commit

I have squashed the commits

dustymabe
dustymabe previously approved these changes Mar 3, 2023
Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

LGTM

We are changing the 'find' invocation permissions to access the
/etc/credstore directory.'find' can't recurse into '/etc/credstore',
we add 'x' so that it can recurse into it. With '+', 'find' accumulates
paths and runs 'chmod' once (or batched by the arg limit). But for
this, we need it to 'chmod' the dir before it tries to recurse into
it, so we change to ';'.

Fixes: coreos/fedora-coreos-tracker#1429
@aaradhak
Copy link
Member Author

aaradhak commented Mar 5, 2023

@dustymabe : there was a W291 (trailing whitespace) error which caused the CI check to fail. Fixed it now. This would need another approval.

@dustymabe dustymabe merged commit 4893e0e into coreos:main Mar 6, 2023
2 checks passed
dustymabe added a commit to dustymabe/fedora-coreos-pipeline that referenced this pull request Mar 6, 2023
This reverts commit 3a94202.

The underlying provlem should have been fixed by
coreos/coreos-assembler#3386
dustymabe added a commit to dustymabe/fedora-coreos-pipeline that referenced this pull request Mar 6, 2023
This reverts commit 3a94202.

The underlying problem should have been fixed by
coreos/coreos-assembler#3386
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.

cosa buildextend-hashlist-experimental fails in branched stream
3 participants