Update and clean up library helpers #101

varshaprasad96 · 2021-08-25T23:32:09Z

This PR does the following:

Updates internal/cmd/run/cmd.go with the changes made to operator flags.
Remove existing example/ repository as the testdata/ folder present in respective plugins folder has the sample operator.
Update scaffolding to use latest versions of controller-runtime and controller-tools.
Cleanup and update a few sections sections of codebase.
Update testdata/ with the changes made to the scaffolding.

The sample testdata present in the PR has been updated with the changes made in the scaffolding code in this PR.

coveralls · 2021-08-25T23:37:08Z

Pull Request Test Coverage Report for Build 1238557586

137 of 144 (95.14%) changed or added relevant lines in 2 files are covered.
4 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.2%) to 89.724%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
internal/flags/flag.go	101	102	99.02%
pkg/watches/watches.go	36	42	85.71%

Files with Coverage Reduction	New Missed Lines	%
pkg/client/actionclient.go	4	78.3%

Totals
Change from base Build 1199182260:	0.2%
Covered Lines:	1493
Relevant Lines:	1664

💛 - Coveralls

varshaprasad96 · 2021-08-26T01:05:46Z

pkg/watches/watches.go

 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"sigs.k8s.io/yaml"
 )

 type Watch struct {
 	schema.GroupVersionKind `json:",inline"`
-	ChartPath               string `json:"chart"`
+	ChartDir                string `json:"chart"`


This is to be consistent with the terminology we use in Helm operator.

/cc @joelanford thoughts on this breaking API change?

I think we accept either a chart directory or a chart tar.gz file, hence the name ChartPath over ChartDir. If we only handle chart directories, this change seems fine.

varshaprasad96 · 2021-08-26T01:08:01Z

pkg/plugins/hybrid/v1alpha/init.go

@@ -153,13 +153,6 @@ func addInitCustomizations(projectName string) error {
 		return err
 	}

-	// Remove the webhook option for the componentConfig since webhooks are not supported by helm


Removing this part of the code which commented out the webhooks out of config files. Since webhooks can be used with Go API, we can leave it here.

fabianvf

/lgtm

I think this all looks good to me, plus tests look like they're running. Probably should hold off on merge until Joe responds about the one breaking change.

Signed-off-by: varshaprasad96 <[email protected]>

internal/cmd/run/cmd.go

pkg/plugins/hybrid/v1alpha/testdata/memcached-operator/Dockerfile

pkg/watches/watches.go

pkg/watches/watches_test.go

joelanford · 2021-09-07T21:28:08Z

pkg/plugins/hybrid/v1alpha/scaffolds/init.go

@@ -46,7 +46,8 @@ const (
 	hybridOperatorVersion = "0.0.1"

 	// helmPluginVersion is the operator-framework/helm-operator-plugin version to be used in the project
-	helmPluginVersion = "dc6c589504b2884e68dff6dc0e5e87ce8c24702f"
+	// This is the SHA of the latest commit on master, which will be used to scaffold the project.


Does this mean every PR that merges to master needs to update this line?

Could we instead inject the value at build time similar to how we inject version information?

See

helm-operator-plugins/pkg/plugins/v1/scaffolds/init.go

Lines 90 to 95 in dc6c589

func mustGetScaffoldVersion() string {

if version.ScaffoldVersion == "" || version.ScaffoldVersion == version.Unknown {

panic("helm-operator scaffold version is unknown; it must be set during build or by importing this plugin via go modules")

}

return version.ScaffoldVersion

}

@joelanford This is really helpful! But right now, the tests configured in Makefile build SDK locally and replace the plugin with the local path to test it out and it breaks since we don't import it as dependency in go.mod. Ill create an issue to track this, so that we can make this modification and change the way test is written too?

That seems reasonable for now.

But I'm curious if this needs to be resolved before the next release. I might not be fully understanding how this works, but it seems like it will never be possible for the commit SHA on this line to match the SHA of the commit that this line is in.

And if that's true, it means that when we go to tag this repo, this line will actually point to an older commit, and users will end up importing the library at that older commit.

I'm also curious if that test will actually test the current commit. It seems like it will always test the commit that's in this line?

Hmm. Yeah, that's true. Can we let this be an input through operator-sdk where we provide the release to scaffold, till its used as a phase 1 plugin? This doesn't seem to be the ideal way to do so, I think.
But on a longer run even with phase 2 plugins this could be an issue because we have the library and plugin in the same place.
Yes, the test uses the current changes committed to scaffold and check if we are able to build an image. Thinking about it, how would we modify the tests to get the current commit and scaffold? If we cannot put the current commit at this place, the generated testdata and tests will always be verifying the previous commit.

pkg/plugins/hybrid/v1alpha/scaffolds/internal/templates/dockerfile.go

Signed-off-by: varshaprasad96 <[email protected]>

varshaprasad96 · 2021-09-14T19:29:37Z

@joelanford @fabianvf I have opened an issue and created a follow up task in jira to capture the work on removing the current test in Makefile. Can we merge this PR if it looks good?

joelanford · 2021-09-15T16:33:51Z

@varshaprasad96 Just the one last question about bumping the envtest k8s version. Otherwise...

/lgtm

joelanford · 2021-09-15T16:34:36Z

Oh also, did you see this comment? #101 (comment)

fabianvf

/lgtm

Pending the last open discussion with Joe

varshaprasad96 · 2021-09-17T17:17:03Z

Merging this PR. Will add a TODO to investigate on having a separate helmConfig to disallow users setting webhooks host, port and cert Dir through controller-runtime config.

github-actions bot added area/deps area/main-binary area/sdk area/testing labels Aug 25, 2021

varshaprasad96 force-pushed the update/lib branch 2 times, most recently from 64e2199 to 2fbbfbd Compare August 26, 2021 01:08

varshaprasad96 commented Aug 26, 2021

View reviewed changes

fabianvf approved these changes Aug 31, 2021

View reviewed changes

openshift-ci bot assigned fabianvf Aug 31, 2021

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

varshaprasad96 added 6 commits September 3, 2021 12:29

update library

2488c14

remove old example

00b264e

Update scaffolding versions

7838b2d

Update test files

c71938f

Signed-off-by: varshaprasad96 <[email protected]>

Update testdata for hybrid helm operator

b18a32d

Signed-off-by: varshaprasad96 <[email protected]>

revert variable in watches.yaml

b953788

varshaprasad96 force-pushed the update/lib branch from 2fbbfbd to b953788 Compare September 3, 2021 19:50

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 3, 2021

github-actions bot added the area/infra label Sep 3, 2021

bump go releaser version

fb4695a

Signed-off-by: varshaprasad96 <[email protected]>

varshaprasad96 force-pushed the update/lib branch from 0200d74 to fb4695a Compare September 3, 2021 19:58

joelanford reviewed Sep 3, 2021

View reviewed changes

joelanford reviewed Sep 7, 2021

View reviewed changes

varshaprasad96 added 3 commits September 9, 2021 13:40

bump c-r versions

427cde0

address review comments

15550ea

fix dockerfile

df1367d

varshaprasad96 force-pushed the update/lib branch from 2e70709 to df1367d Compare September 9, 2021 22:17

varshaprasad96 added 2 commits September 9, 2021 16:10

Additional tests for watches

9ded764

Modify gid while copying file in dockerfile

7a0b06b

varshaprasad96 requested a review from joelanford September 9, 2021 23:13

joelanford reviewed Sep 10, 2021

View reviewed changes

pkg/plugins/hybrid/v1alpha/scaffolds/internal/templates/dockerfile.go Outdated Show resolved Hide resolved

make modification to dockerfile

1919ebf

Signed-off-by: varshaprasad96 <[email protected]>

varshaprasad96 force-pushed the update/lib branch from 99f53ea to 1919ebf Compare September 13, 2021 16:40

varshaprasad96 requested a review from joelanford September 13, 2021 19:13

varshaprasad96 mentioned this pull request Sep 14, 2021

Framework for testing helm and hybrid plugins in this repository #104

Open

openshift-ci bot assigned joelanford Sep 15, 2021

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

bump envtest k8s binaries to 1.22

1302d1d

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 15, 2021

fabianvf approved these changes Sep 15, 2021

View reviewed changes

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

varshaprasad96 merged commit c5ecef2 into operator-framework:main Sep 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update and clean up library helpers #101

Update and clean up library helpers #101

varshaprasad96 commented Aug 25, 2021

coveralls commented Aug 25, 2021 •

edited

Loading

varshaprasad96 Aug 26, 2021

fabianvf Aug 31, 2021

joelanford Sep 3, 2021

varshaprasad96 Aug 26, 2021

fabianvf left a comment

joelanford Sep 7, 2021

joelanford Sep 7, 2021

varshaprasad96 Sep 9, 2021 •

edited

Loading

joelanford Sep 10, 2021

varshaprasad96 Sep 10, 2021

varshaprasad96 commented Sep 14, 2021 •

edited

Loading

joelanford commented Sep 15, 2021

joelanford commented Sep 15, 2021

fabianvf left a comment

varshaprasad96 commented Sep 17, 2021

	func mustGetScaffoldVersion() string {
	if version.ScaffoldVersion == "" \|\| version.ScaffoldVersion == version.Unknown {
	panic("helm-operator scaffold version is unknown; it must be set during build or by importing this plugin via go modules")
	}
	return version.ScaffoldVersion
	}

Update and clean up library helpers #101

Update and clean up library helpers #101

Conversation

varshaprasad96 commented Aug 25, 2021

coveralls commented Aug 25, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1238557586

💛 - Coveralls

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

fabianvf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varshaprasad96 Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varshaprasad96 commented Sep 14, 2021 • edited Loading

joelanford commented Sep 15, 2021

joelanford commented Sep 15, 2021

fabianvf left a comment

Choose a reason for hiding this comment

varshaprasad96 commented Sep 17, 2021

coveralls commented Aug 25, 2021 •

edited

Loading

varshaprasad96 Sep 9, 2021 •

edited

Loading

varshaprasad96 commented Sep 14, 2021 •

edited

Loading