Skip to content

pkg/metadata: bundle metadata utils and types#60

Closed
estroz wants to merge 3 commits intooperator-framework:masterfrom
estroz:feature/find-metadata
Closed

pkg/metadata: bundle metadata utils and types#60
estroz wants to merge 3 commits intooperator-framework:masterfrom
estroz:feature/find-metadata

Conversation

@estroz
Copy link
Copy Markdown
Member

@estroz estroz commented Aug 12, 2020

There should be a canonical set of utilities and types that can find and parse an annotations.yaml in a bundle directory instead of re-implementing that logic in various ways in various operator-framework repos. This repo is a good home for them because it is the source of truth for many OF APIs.

/cc @kevinrizza @dinhxuanvu

/kind feature

@openshift-ci-robot openshift-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Aug 12, 2020
@estroz estroz force-pushed the feature/find-metadata branch from 262762c to 8b174fd Compare August 12, 2020 19:58
@estroz
Copy link
Copy Markdown
Member Author

estroz commented Aug 12, 2020

Relevant to this PR: because we're adding this function to the api lib, can we consider adding the bundle spec itself to this lib, specifically labels and some convenient access methods for them?

Comment thread pkg/metadata/annotations.go Outdated
// readAnnotations reads annotations from file(s) in bundleRoot and returns them as a map.
func readAnnotations(fs afero.Fs, annotationsPath string) (map[string]string, error) {
// The annotations file is well-defined.
b, err := afero.ReadFile(fs, annotationsPath)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not just use ioutil instead of adding an external dependency?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's much easier to test with afero since we don't have to write testdata to disk for each new case. Plus there's a Go design draft for a std file system interface, which we could replace this with in the future.

Comment thread pkg/metadata/annotations.go Outdated

// Use the arbitrarily-indexed representation of the annotations file for forwards and backwards compatibility.
annotations := struct {
Annotations map[string]string `json:"annotations"`
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why don't we use a more versioned structured type here? Just so that additional arbitrary annotations can be added? It seems like either way we need to validate that it has the set of required annotations to make the bundle work, so maybe that should be included here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Exactly, so arbitrary annotations can be added. There may be a way to load both versioned and arbitrary annotations into some struct type, which I can experiment with in this PR, but I wasn't sure about doing that initially (see #60 (comment)).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done

and return the full AnnotationsFile from FindAnnotations()

pkg/manifests: move metadata types to pkg/metadata
Comment thread pkg/metadata/directory.go
"strings"

log "github.com/sirupsen/logrus"
"github.com/spf13/afero"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nice find!

@estroz estroz changed the title pkg/metadata: add annotations.yaml search utility function FindAnnotations() pkg/metadata: bundle metadata utils and types Aug 13, 2020
Comment thread pkg/metadata/types.go Outdated
Copy link
Copy Markdown
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

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

/lgtm
Just a small nit.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 20, 2020
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 20, 2020
Copy link
Copy Markdown
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 20, 2020
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 11, 2021
@openshift-ci-robot
Copy link
Copy Markdown

@estroz: PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@bcrochet
Copy link
Copy Markdown
Contributor

Is this still alive? I could sure use it. I'd be happy to rebase it and get it updated.

@dinhxuanvu
Copy link
Copy Markdown
Member

Is this still alive? I could sure use it. I'd be happy to rebase it and get it updated.

Sure. Go ahead.

@perdasilva
Copy link
Copy Markdown
Contributor

closing as stale

@perdasilva perdasilva closed this Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants