Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(opm): add unprivileged registry add #213
feat(opm): add unprivileged registry add #213
Changes from 1 commit
8ae4669
d05586a
f666f9d
a5f3ef3
207a140
3067694
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should consider making this a hidden flag in case anyone was relying on it existing (instead of throwing an error about not knowing the flag).
If we're confident no one is relying on it yet, we can remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benluddy and I were discussing this in an earlier review. Within Red Hat, I know there's some unreleased automation making use of the default value (not specifying
--container-tool
). However, I do think it's safer to use a hidden flag. Can we meet in the middle with a hidden flag that logs a warning when specified and continues with the daemonless implementation?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made it a deprecated flag, which is both hidden and gives a warning when used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there any way to resolve this? we just got these all cleaned up...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could just not run an in-process docker registry, which I expect would make the tests more complex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also a test dependency, which I think means it's not required by dependents to use the operator-registry module.
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I like is using custom types for specificity such a
type ImageReference string
and then in the interface it would take in aref ImageReference
type as the arg. I'm not sure how appropriate it is here but something worth noting as it makes the code more readable IMO.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't insist on it, but I also appreciate the extra compile-time safety of derived types for this kind of thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay -- I'll change this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least we got bolt in here somehow!
We don't expect to have endianness concerns with this because this is a generated always at runtime and never persisted, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, it's always generated at runtime unless you really want to use an existing file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
os.Getuid
andos.Getgid
return -1 on Windows, are there any portability implications?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch. I think there could be and I'm fairly certain
opm
doesn't work on windows as it stands. There are already multiple failures in the travis windows tests (our config just isn't picking them up).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's handy!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to defer this to increase the odds it happens on test failures, unless this was an intentional decision to help debug failures?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was intentional, but I can put it in a defer.
Large diffs are not rendered by default.