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

feat(operator): catalog metadata types for prior index data #29

Merged
merged 1 commit into from
Aug 11, 2021
Merged

feat(operator): catalog metadata types for prior index data #29

merged 1 commit into from
Aug 11, 2021

Conversation

estroz
Copy link
Contributor

@estroz estroz commented Aug 6, 2021

This PR adds the ability to store and pull image set metadata from a backend, and fixes some create metadata handling issues. Currently only a local directory storage backend is supported.

Signed-off-by: Eric Stroczynski ericstroczynski@gmail.com

@estroz estroz requested review from afflom and jpower432 August 6, 2021 22:19
@estroz estroz changed the title [WIP] operator catalog metadata feat(operator): catalog metadata types for prior index data Aug 9, 2021
@estroz
Copy link
Contributor Author

estroz commented Aug 9, 2021

Blocking on #28 so I can rebase.

return err
case err == nil && len(meta.PastMirrors) != 0:
// No past run(s) are allowed when generating a full imageset.
// QUESTION: should the command just log a warning and ignore this metadata instead?
Copy link
Contributor

Choose a reason for hiding this comment

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

@estroz Responding to your comment in the code, would it better to prompt the user or would we send a warning when creating a full and destroy the current metadata. @afflom and I talked about this and the user should be able to create a full even if there is existing metadata, but the only area of concern I have about this is the user accidentally running create full. Maybe a prompt or a metadata backup? WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Prompt sounds fine. I'll create a follow-up.

Copy link
Contributor

@jpower432 jpower432 left a comment

Choose a reason for hiding this comment

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

LGTM, but needs a change for the diff Uid to maintain the same Uid between runs. We may want to also discuss how we are going to handle new create full runs on currently managed data.

pkg/bundle/create/create.go Outdated Show resolved Hide resolved
This commit also hoists the Uid field to the top-level MetadataSpec
object, since it identifies the whole metadata object and not just
an individual run.

Signed-off-by: Eric Stroczynski <ericstroczynski@gmail.com>
@estroz estroz requested a review from jpower432 August 11, 2021 17:59
if err != nil {
return fmt.Errorf("creating %s: %v", nameInArchive, err)
}
if _, found := foundFiles[fpath]; !found {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just moved the actual archive logic left and indent, and localized the skip logic. No semantic changes made other than skipping . explicit.

Copy link
Contributor

@jpower432 jpower432 left a comment

Choose a reason for hiding this comment

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

/lgtm

@estroz estroz merged commit 0ab8fb1 into openshift:main Aug 11, 2021
@estroz estroz deleted the feat/operator-metadata branch August 11, 2021 18:44
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.

Fix: Change GetAdditional Test to resolve issues with hitting Docker rate limiting
3 participants