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

Zero Descriptor UID/GID (V2) #61

Merged
merged 4 commits into from
Jul 15, 2021
Merged

Zero Descriptor UID/GID (V2) #61

merged 4 commits into from
Jul 15, 2021

Conversation

tri-adam
Copy link
Member

@tri-adam tri-adam commented Jul 15, 2021

Mark UID/GID fields deprecated in type Descriptor struct, and set values to zero. Remove UID/GID values from info command output. Update integrity package tests to begin with zero value UID/GID. Don't consider deprecated UID/GID fields in GetFromDescr.

Closes #59

Force UID/GID values to zero.
@tri-adam tri-adam self-assigned this Jul 15, 2021
@codecov-commenter
Copy link

codecov-commenter commented Jul 15, 2021

Codecov Report

Merging #61 (f7fe0ed) into master (08f0698) will increase coverage by 0.34%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #61      +/-   ##
==========================================
+ Coverage   69.82%   70.16%   +0.34%     
==========================================
  Files          25       25              
  Lines        1922     1904      -18     
==========================================
- Hits         1342     1336       -6     
+ Misses        390      384       -6     
+ Partials      190      184       -6     
Impacted Files Coverage Δ
pkg/sif/fmt.go 71.08% <ø> (-0.69%) ⬇️
pkg/sif/lookup.go 69.36% <ø> (+1.22%) ⬆️
pkg/sif/sif.go 31.18% <ø> (ø)
pkg/sif/create.go 54.54% <100.00%> (+0.76%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08f0698...f7fe0ed. Read the comment docs.

Adam Hughes added 3 commits July 15, 2021 15:22
These are now always set to zero, and marked as deprecated. Remove from
human-readable output to reflect the change.
Update to reflect deprecation of UID/GID fields. Keeping the UID/GID
tests, since these are still relevant in terms of descriptor integrity.
@dtrudg
Copy link
Member

dtrudg commented Jul 15, 2021

Since 0 infers root for uid/gid fields... would a -1 be acceptable here as a 'not used' value that distinguishes from 0 that might be present in older images.. given they are int and not uint definitions here?

@tri-adam
Copy link
Member Author

Since 0 infers root for uid/gid fields... would a -1 be acceptable here as a 'not used' value that distinguishes from 0 that might be present in older images.. given they are int and not uint definitions here?

We could, although it's worth noting that the v1 API does use 0 to mean unset/ignore in at least one context (searching for descriptors):

sif/pkg/sif/lookup.go

Lines 227 to 232 in 08f0698

if descr.UID != 0 && descr.UID != v.UID {
continue
}
if descr.GID != 0 && descr.GID != v.GID {
continue
}

I have a slight preference for keeping it zero value, for no other reason than it being Go's default value, which reduces the number of references required to deprecated fields in tests that simulate the default behaviour (for example in the integrity tests that currently utilize the Descriptor type directly.)

Does that make sense?

@tri-adam tri-adam changed the title Zero Descriptor UID/Gid (V2) Zero Descriptor UID/GID (V2) Jul 15, 2021
@tri-adam tri-adam marked this pull request as ready for review July 15, 2021 16:13
@dtrudg
Copy link
Member

dtrudg commented Jul 15, 2021

Since 0 infers root for uid/gid fields... would a -1 be acceptable here as a 'not used' value that distinguishes from 0 that might be present in older images.. given they are int and not uint definitions here?

We could, although it's worth noting that the v1 API does use 0 to mean unset/ignore in at least one context (searching for descriptors):

sif/pkg/sif/lookup.go

Lines 227 to 232 in 08f0698

if descr.UID != 0 && descr.UID != v.UID {
continue
}
if descr.GID != 0 && descr.GID != v.GID {
continue
}

I have a slight preference for keeping it zero value, for no other reason than it being Go's default value, which reduces the number of references required to deprecated fields in tests that simulate the default behaviour (for example in the integrity tests that currently utilize the Descriptor type directly.)

Does that make sense?

Fair enough. It is one of those areas where it's pretty awkward that the default value is a meaningful one here, but this doesn't seem to have been used anywhere.

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.

Deprecate Descriptor UID/GID
3 participants