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

Allow setting ownership by name #1541

Merged
merged 3 commits into from
Mar 14, 2022
Merged

Conversation

lukas-w
Copy link
Contributor

@lukas-w lukas-w commented Feb 5, 2022

As proposed in #1531 (comment), this changes the new file ownership specification from integer uid/gid to string user/group that can either be a decimal id or a user/group name. This is both for convenience and for future compatibility with Windows. This change allows us to implement Windows support in the future without changing the public interface in consul-template and Nomad. One catch in the context of Nomad is that user/group names would be looked up on the host, not e.g. inside the Docker container as users may expect.

I also fixed a potential nil dereference via af770ec and made Render(...) fail when trying to set file ownership on Windows analogous to how it fails if Chmod fails via 5be1f70.

Tagging @eikenb because they kindly offered to review.

Introduced in 24cb142: An error in
os.Stat would lead to a nil pointer dereference in the calling function
@lukas-w lukas-w requested a review from a team February 5, 2022 08:37
@eikenb
Copy link
Contributor

eikenb commented Feb 8, 2022

Hey @lukas-w, nice PR.

I've looked it over and everything looks good. I want to pull it down and check it over locally to be sure, but I think I'll be able to merge it as is and it should make it into 0.28.0. Thanks!

@eikenb eikenb added this to the v0.28.0 milestone Feb 8, 2022
@eikenb eikenb modified the milestones: v0.28.0, v0.29.0 Mar 3, 2022
@hashicorp-cla
Copy link

hashicorp-cla commented Mar 12, 2022

CLA assistant check
All committers have signed the CLA.

@igor-nikiforov
Copy link

@eikenb I'm very sorry for ping you but there is any ETA when this PR will be merged?

@eikenb
Copy link
Contributor

eikenb commented Mar 14, 2022

Sorry, 0.28.0 went out a bit early due to a customer need which bumped this to 0.29 and distracted me for a bit. I was able to pull it down and look it over though and it still looks good. In other words the answer is...

Thanks for reminding me, I'll merge it in a few minutes.

@eikenb
Copy link
Contributor

eikenb commented Mar 14, 2022

Just realized an issue. This changes the behavior in a backwards incompatible way. It wasn't an issue before as it was going in the same release as the original change and so there was no release. Sigh.

I think I'm just going to keep it as a backwards incompatible change (mentioning it in the changelog) as that was the intention before the early release.

@deblasis, if you happen to see this and wouldn't mind, would you chime in with your thoughts on this. I'm going to merge it but could add backwards compatibility to it after the fact if there is a need. Thanks.

@eikenb eikenb merged commit 99c90a7 into hashicorp:master Mar 14, 2022
@eikenb eikenb added the breaking-change This item needs a breaking-change CHANGELOG entry label Mar 14, 2022
@lukas-w lukas-w deleted the chown-by-name branch March 15, 2022 09:24
@deblasis
Copy link
Contributor

Hi @eikenb !

Yeah, I think we still have to support uid and gid settings while keeping user and group of course.

I found a couple of minutes and I put together #1551

Open to suggestions/improvements if needed as always

@eikenb eikenb removed the breaking-change This item needs a breaking-change CHANGELOG entry label Mar 21, 2022
@eikenb
Copy link
Contributor

eikenb commented Mar 21, 2022

Removed the breaking-change label as that was mitigated by #1551

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants