Adds support for img labels and anno. in build create cmd #68

sm43 · 2021-10-29T10:39:41Z

This adds support to specify labels and annotation for
output image.
There are 2 flags added for the build create command

output-image-label
output-image-annotation

Part of #66

Signed-off-by: Shivam Mukhade [email protected]

Changes

Submitter Checklist

Includes tests if functionality changed/was added
Includes docs if changes are user-facing
Set a kind label on this PR
Release notes block has been filled in, or marked NONE

See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.

Release Notes

Adds support to specify on Build objects labels and annotations to be set on output images

sm43 · 2021-10-29T10:39:58Z

/cc @otaviof

SaschaSchwarze0

Thank you, solid work. Please correct the handling of values with none or multiple = characters.

pkg/shp/util/env.go

SaschaSchwarze0

Thank you for the update, looks good now for me. One more small change to the flag name.

pkg/shp/flags/flags.go

gabemontero · 2021-10-29T13:14:04Z

hey @sm43 - thanks for the contribution

per our PR template at https://github.com/shipwright-io/cli/blob/main/.github/pull_request_template.md we have PR authors craft the release note that should go along with their change

ultimately, when my PR #62 merges, our CI will enforce the creation of the release note. But in the interim, and given you are new the project, I took the liberty of assisting you and modifying your PR description for you. Please take a look, and if you think the release note needs modification, please do edit your description accordingly.

I'll next take a peek at your changes

thanks again

SaschaSchwarze0

/approve
/assign gabemontero

Nice work. I have to admit that I do not know the e2e test framework from this repo good enough. Leaving it to Gabe for a final assessment.

openshift-ci · 2021-10-29T13:22:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SaschaSchwarze0

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [SaschaSchwarze0]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

gabemontero

ideally we've have e2e tests to go along with this, but I'm good either opening a follow up issue for that, or taking that on if the buildrun stuff does in fact happen soonish

otherwise, a couple of minor suggestions on the help text

pkg/shp/flags/flags.go

sm43 · 2021-10-30T04:07:10Z

@gabemontero Thanks for release notes 🙂 will keep in mind to fill it from next time.
e2e framework used is new for me so will address the e2e test in a follow up PR.

sm43 · 2021-11-02T04:11:56Z

/hold
will try adding e2e!

sm43 · 2021-11-02T04:33:58Z

/hold cancel
@gabemontero added a e2e test, ptal.

otaviof

I would rather pursue the pflag.Value approach, so we can keep flags package responsible for providing a BuildSpec/BuildRunSpec without side effects, improved testability and more.

Please consider the changes in #69, and the example usage of flags package.

This adds support to specify labels and annotation for output image. There are 2 flags added for the `build create` command - output-image-label - output-image-annotation Signed-off-by: Shivam Mukhade <[email protected]>

sm43 · 2021-11-02T16:16:33Z

I would rather pursue the pflag.Value approach, so we can keep flags package responsible for providing a BuildSpec/BuildRunSpec without side effects, improved testability and more.

Please consider the changes in #69, and the example usage of flags package.

Thanks @otaviof updated the pr.
cc @gabemontero

otaviof

Aside of nit comments, it looks good!

/lgtm

otaviof · 2021-11-02T17:47:15Z

pkg/shp/flags/map_value_test.go

+	spec := &buildv1alpha1.BuildSpec{Output: buildv1alpha1.Image{
+		Labels: map[string]string{},
+	}}
+	c := NewMapValue(spec.Output.Labels)


nit: rename this variable to m it follows the original type.

otaviof · 2021-11-02T17:47:42Z

test/e2e/output-image-labels-annotations.bats

+	run kubectl delete buildruns.shipwright.io --all
+}
+
+@test "shp output image labels and annotation lifecycle" {


great, thank you!

otaviof · 2021-11-02T17:48:34Z

pkg/shp/flags/map_value.go

+// Type analogous to the pflag "stringArray" type, where each flag entry will be translated to a
+// single array (slice) entry, therefore the comma (",") is accepted as part of the value, as any
+// other special character.
+func (c *MapValue) Type() string {


nit: rename c to _, since it's not in use.

otaviof · 2021-11-02T17:49:00Z

pkg/shp/flags/flags.go

+		"",
+		"specify a set of key-value pairs that correspond to labels to set on the output image",
+	)
+


nit: remove empty line.

openshift-ci bot added the release-note label Oct 29, 2021

openshift-ci bot requested review from adambkaplan and qu1queee October 29, 2021 10:39

openshift-ci bot requested a review from otaviof October 29, 2021 10:39

sm43 force-pushed the build-cmd branch from 2af4063 to f723cb0 Compare October 29, 2021 10:50

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 29, 2021

sm43 force-pushed the build-cmd branch from f723cb0 to 7250bae Compare October 29, 2021 10:51

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 29, 2021

sm43 changed the title ~~Adds support to specify image labels and annotations~~ Adds support for img labels and anno. in build create cmd Oct 29, 2021

SaschaSchwarze0 requested changes Oct 29, 2021

View reviewed changes

pkg/shp/util/env.go Outdated Show resolved Hide resolved

SaschaSchwarze0 added the kind/feature Categorizes issue or PR as related to a new feature. label Oct 29, 2021

sm43 force-pushed the build-cmd branch from 7250bae to 5df9ab9 Compare October 29, 2021 12:25

otaviof added the hackday label Oct 29, 2021

SaschaSchwarze0 requested changes Oct 29, 2021

View reviewed changes

pkg/shp/flags/flags.go Outdated Show resolved Hide resolved

sm43 force-pushed the build-cmd branch from 5df9ab9 to c95b92c Compare October 29, 2021 12:46

SaschaSchwarze0 approved these changes Oct 29, 2021

View reviewed changes

openshift-ci bot assigned gabemontero Oct 29, 2021

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 29, 2021

gabemontero requested changes Oct 29, 2021

View reviewed changes

pkg/shp/flags/flags.go Outdated Show resolved Hide resolved

pkg/shp/flags/flags.go Outdated Show resolved Hide resolved

sm43 force-pushed the build-cmd branch from c95b92c to cee09d0 Compare October 30, 2021 04:03

sm43 mentioned this pull request Oct 30, 2021

Fixes create cmds to return an error if env passed is invalid #72

Merged

4 tasks

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 1, 2021

sm43 force-pushed the build-cmd branch from cee09d0 to 956f8e7 Compare November 1, 2021 04:27

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 1, 2021

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 2, 2021

sm43 force-pushed the build-cmd branch from 24ca414 to 32cf130 Compare November 2, 2021 04:25

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 2, 2021

sm43 force-pushed the build-cmd branch from 32cf130 to 63693a2 Compare November 2, 2021 04:25

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 2, 2021

sm43 force-pushed the build-cmd branch from 63693a2 to db7ee09 Compare November 2, 2021 04:29

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 2, 2021

otaviof requested changes Nov 2, 2021

View reviewed changes

sm43 force-pushed the build-cmd branch from db7ee09 to 062b595 Compare November 2, 2021 16:05

Adds support for img labels and anno. in build create cmd

3522d4e

This adds support to specify labels and annotation for output image. There are 2 flags added for the `build create` command - output-image-label - output-image-annotation Signed-off-by: Shivam Mukhade <[email protected]>

sm43 force-pushed the build-cmd branch from 062b595 to 3522d4e Compare November 2, 2021 16:10

otaviof reviewed Nov 2, 2021

View reviewed changes

openshift-ci bot assigned otaviof Nov 2, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 2, 2021

openshift-merge-robot merged commit f1930e7 into shipwright-io:main Nov 2, 2021

sm43 mentioned this pull request Nov 21, 2021

Adds support for img labels and anno. in buildrun create cmd #76

Merged

4 tasks

SaschaSchwarze0 added this to the release-v0.7.0 milestone Dec 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for img labels and anno. in build create cmd #68

Adds support for img labels and anno. in build create cmd #68

sm43 commented Oct 29, 2021 •

edited

Loading

sm43 commented Oct 29, 2021

SaschaSchwarze0 left a comment

SaschaSchwarze0 left a comment

gabemontero commented Oct 29, 2021

SaschaSchwarze0 left a comment

openshift-ci bot commented Oct 29, 2021

gabemontero left a comment

sm43 commented Oct 30, 2021

sm43 commented Nov 2, 2021

sm43 commented Nov 2, 2021

otaviof left a comment

sm43 commented Nov 2, 2021

otaviof left a comment

otaviof Nov 2, 2021

otaviof Nov 2, 2021

otaviof Nov 2, 2021

otaviof Nov 2, 2021

Adds support for img labels and anno. in build create cmd #68

Adds support for img labels and anno. in build create cmd #68

Conversation

sm43 commented Oct 29, 2021 • edited Loading

Changes

Submitter Checklist

Release Notes

sm43 commented Oct 29, 2021

SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

gabemontero commented Oct 29, 2021

SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 29, 2021

gabemontero left a comment

Choose a reason for hiding this comment

sm43 commented Oct 30, 2021

sm43 commented Nov 2, 2021

sm43 commented Nov 2, 2021

otaviof left a comment

Choose a reason for hiding this comment

sm43 commented Nov 2, 2021

otaviof left a comment

Choose a reason for hiding this comment

otaviof Nov 2, 2021

Choose a reason for hiding this comment

otaviof Nov 2, 2021

Choose a reason for hiding this comment

otaviof Nov 2, 2021

Choose a reason for hiding this comment

otaviof Nov 2, 2021

Choose a reason for hiding this comment

sm43 commented Oct 29, 2021 •

edited

Loading