Refactoring corev1.EnvVar Flag using pflag.Value #69

otaviof · 2021-10-29T13:40:44Z

Changes

Implementing pflag.Value interface to represent corev1.EnvVar slice, used in both Build and BuildRun instances.

The objective is to avoid any side effects of using BuildSpecFromFlags and BuildRunSpecFromFlags, those methods should provide a ready-to-use BuildSpec/BuildRunSPec instance, so consumers won't need to complete them outside of the flags package scope.

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

Release Notes

NONE

otaviof · 2021-10-29T13:52:46Z

/assign @coreydaley @SaschaSchwarze0
/cc @gabemontero @adambkaplan

adambkaplan

/approve

Adding mainly style improvements/suggestions. Othwise signaling that this is a good improvement and can be LGTM-ed by another member of the community.

pkg/shp/flags/csv.go

pkg/shp/flags/csv_test.go

openshift-ci · 2021-10-29T14:01:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan

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~~ [adambkaplan]

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

otaviof · 2021-10-29T14:10:16Z

/approve

Adding mainly style improvements/suggestions. Othwise signaling that this is a good improvement and can be LGTM-ed by another member of the community.

Thanks!

renatocron · 2021-10-29T16:10:31Z

pkg/shp/flags/csv.go

+// splitKeyValue splits a key-value string with the format "key=value". Returns the key, value, and
+// an error if the string is not formatted correctly.
+func splitKeyValue(value string) (string, string, error) {
+	s := strings.Split(value, "=")


There's is some encoding here? Or should it be SplitN(value, "=", 2) to allow "=" to be on values

Good catch! Definitely we need to allow a single equal (=) sign to define the variables. I've updated this part with your suggestion, and included a extra test case to cover the specific scenario. Please consider :-)

SaschaSchwarze0

Interesting change.

SaschaSchwarze0 · 2021-10-31T18:56:34Z

pkg/shp/flags/core_envvar_value.go

+
+// Set receives a string with key-value pairs comma separated.
+func (c *CoreEnvVarSliceValue) Set(value string) error {
+	values, err := readAsCSV(value)


Imo we should generally not do this. For multi-value fields we should only support the way where the argument is provided multiple times:

-env key1=value1 -env key2=value2

The reason is that this allows to support , in the value without the need for any escaping.

Thanks @SaschaSchwarze0, indeed we need to handle each entry without having to parse CSV. I've changed the implementation to use stringArray instead of stringSlice, so we have have the desired behavior you explained, please consider.

SaschaSchwarze0 · 2021-10-31T19:01:40Z

pkg/shp/flags/core_envvvar_value_test.go

@@ -0,0 +1,35 @@
+package flags


In the file name, you have three subsequent vs where I think only two are desired.

Indeed, I renamed in the latest set of changes. Thanks!

SaschaSchwarze0 · 2021-10-31T19:02:23Z

pkg/shp/flags/core_envvar_value.go

@@ -0,0 +1,61 @@
+package flags


Copyright header missing, though I just notice that many existing files also do not contain one.

I think we still need to bring some tools from the build-controller to this repository as well 🤔

Yep, opened Ensure files have copyright headers #73.

Implementing pflag.Value interface to represent corev1.EnvVar used in both Build and BuildRun. Co-authored-by: Adam Kaplan <[email protected]>

SaschaSchwarze0

/lgtm

SaschaSchwarze0 · 2021-11-02T08:14:10Z

pkg/shp/flags/core_envvar_value.go

@@ -0,0 +1,61 @@
+package flags


Yep, opened Ensure files have copyright headers #73.

otaviof added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. hackday labels Oct 29, 2021

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

openshift-ci bot requested review from adambkaplan and SaschaSchwarze0 October 29, 2021 13:40

openshift-ci bot assigned coreydaley and SaschaSchwarze0 Oct 29, 2021

openshift-ci bot requested a review from gabemontero October 29, 2021 13:52

otaviof force-pushed the hackday-envvar-flag branch from e8a6eef to e1035d9 Compare October 29, 2021 13:58

adambkaplan reviewed Oct 29, 2021

View reviewed changes

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

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

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

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

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

otaviof force-pushed the hackday-envvar-flag branch from 0d3561e to 116a375 Compare October 29, 2021 14:09

renatocron reviewed Oct 29, 2021

View reviewed changes

otaviof force-pushed the hackday-envvar-flag branch 3 times, most recently from 62129f3 to 0471db5 Compare October 30, 2021 05:08

SaschaSchwarze0 requested changes Oct 31, 2021

View reviewed changes

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

Refactoring corev1.EnvVar Flag

1a1d2d8

Implementing pflag.Value interface to represent corev1.EnvVar used in both Build and BuildRun. Co-authored-by: Adam Kaplan <[email protected]>

otaviof force-pushed the hackday-envvar-flag branch from 0471db5 to 1a1d2d8 Compare November 2, 2021 07:11

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

otaviof requested review from SaschaSchwarze0 and adambkaplan November 2, 2021 07:33

SaschaSchwarze0 approved these changes Nov 2, 2021

View reviewed changes

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

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

sm43 mentioned this pull request Nov 2, 2021

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

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

Refactoring corev1.EnvVar Flag using pflag.Value #69

Refactoring corev1.EnvVar Flag using pflag.Value #69

otaviof commented Oct 29, 2021 •

edited

Loading

otaviof commented Oct 29, 2021

adambkaplan left a comment

openshift-ci bot commented Oct 29, 2021

otaviof commented Oct 29, 2021

renatocron Oct 29, 2021

otaviof Oct 30, 2021 •

edited

Loading

SaschaSchwarze0 left a comment

SaschaSchwarze0 Oct 31, 2021

otaviof Nov 2, 2021

SaschaSchwarze0 Oct 31, 2021

otaviof Nov 2, 2021

SaschaSchwarze0 Oct 31, 2021

otaviof Nov 2, 2021

SaschaSchwarze0 Nov 2, 2021

SaschaSchwarze0 left a comment

SaschaSchwarze0 Nov 2, 2021

Refactoring corev1.EnvVar Flag using pflag.Value #69

Refactoring corev1.EnvVar Flag using pflag.Value #69

Conversation

otaviof commented Oct 29, 2021 • edited Loading

Changes

Submitter Checklist

Release Notes

otaviof commented Oct 29, 2021

adambkaplan left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 29, 2021

otaviof commented Oct 29, 2021

Choose a reason for hiding this comment

otaviof Oct 30, 2021 • edited Loading

Choose a reason for hiding this comment

SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otaviof commented Oct 29, 2021 •

edited

Loading

otaviof Oct 30, 2021 •

edited

Loading