[SDK] - Proposal for samples generation via GO and in the SDK repo #47

camilamacedo86 · 2020-09-04T14:52:52Z

NOTE: It has images and code example, so I'd suggest doing the review by seen the preview

estroz

A few problems with this PR:

It mostly follows the EP template, but the Proposal section does not in a detrimental manner. Where are the implementation notes/details?
The EP is very light on technical detail. How are code changes to be made, ex. we update part of the reconcile loop? What is a SampleContext (there is a good example of one, but no discussion about its semantics in English).
There is no discussion as to how this will be tied into the SDK's release process, which is tangential to the generator's implementation but still relevant to this EP.

enhancements/samples-generation.md

camilamacedo86 · 2020-09-05T00:27:11Z

Hi @estroz,

Really thank you for your time and input. I will ping you when I finish it.

camilamacedo86 · 2020-09-05T13:48:08Z

Hi @estroz,

Please, feel free to re-check it. really thank you for your time.

varshaprasad96 · 2020-10-01T17:58:40Z

enhancements/samples-generation.md

+
+## Open Questions
+
+1. Should not the project has a Memcached GO Sample with webhooks and other without using webhooks since it shows an advanced feature and it used is not mandatory? Note the Go tutorial do not use webhooks.  


Would we be having 2 samples. One for using web hooks with cert-manager (which is already present upstream), and WH implementation with OLM ?

Yes, we would have 2 samples. However, in upstream/kb we have none similar testdata. The idea is we have samples with the steps/implementation suggested to users in the tutorials. The Go Memcached tutorial has none webhooks and we ought to have an advanced tutorial with the additional steps to use webhooks. So, in the end, it would be:

// The Memcached sample without webhooks
// The Memcached sample with webhooks

Note that we can re-use the first sample steps as we will do in the e2e test to build the second one.
Also, both should be integrated with OLM since it is a key feature of the tool.

varshaprasad96 · 2020-10-01T18:06:23Z

enhancements/samples-generation.md

+
+### Implementation Details/Notes/Constraints 
+
+- The samples should be build inside of `./testdata/<type>/` path


Just a quick doubt. Im not sure how KB does this. But, if we have different plugins for same type of operator, would we scaffold the project with default plugin version which is set.

At the moment we have only one version of the plugin. To create the samples we use the SDK binary and run the commands at the same way that users will do that. So, in the future, if we wish we can create samples for diff plugins versions since it means operator-sdk --plugins="plugin version"

@varshaprasad96 @camilamacedo86 I would NOT scaffold out samples with multiple plugins. I would create the samples for our default use case only. And possibly for any complex use cases like we are talking about with webhooks. Honestly I can see 2 samples: 1) default easy peasy project. 2) one with all the options enabled to show the complex features.

@jmrodri it is not in the scope of the EP anyway. However, the stack allows us to do that if we wish in the future. I mean, has no limitation for that if we wish to do that. But I agree 100% with you.

varshaprasad96 · 2020-10-01T18:10:10Z

enhancements/samples-generation.md

+
+#### Generate samples via Go 
+
+- I am a maintainer/contributor, I want to be able to troubleshoot the code used to create the samples, so that I can easily fix the issues. 


If we are going to scaffold the operator from scratch and test all the functionalities, then would we be removing the existing e2e tests for go/ansible/helm? I see that you have mentioned e2e tests would invoke this, but wouldn't we be doing the same testing twice if we have both.

Using the samples for the e2e tests is an idea on the table, but when we talked about this initially we thought that the e2e tests should be out of scope for this proposal to keep it from becoming a huge task.

We will not remove the e2e tests. We will just replace the same/identical steps to mock the project by the call. The additional mock data and checks still as it is now.

To call in the e2e test

By("running samples steps") ctx, err := samplespkg.NewSampleContextWith(&tc) Expect(err).Should(Succeed()) sample := sampleshelm.NewMemcachedHelm(&ctx) sample.Run()

See the above example in the EP : https://github.com/operator-framework/enhancements/pull/47/files#diff-3c2574643214da41adcb10b9a5b8bcd2R336-R344:

enhancements/samples-generation.md

Co-authored-by: Austin Macdonald <austin@redhat.com>

camilamacedo86 · 2020-10-02T14:17:04Z

Hi @varshaprasad96 and @asmacdo,

Really thank you for your review. The typo suggestions are all accepted and the questions raised answered.
Please, let me know if you have anything else that could be addressed here for we get it merged.

jmrodri

/lgtm

**Description of the change:** - Implementation for we are able to auto generated samples with its utils cleanup - Memcached Helm sample generate (NOTE): The usage of the sample in the e2e tests will be made in a follow-up and are not part of the context of this PR. We need just the e2e tests since the samples have been gen using the chart when in the e2e tests are done with another scenario where no chart is informed. **Motivation for the change:** operator-framework/enhancements#47

**Description of the change:** Add Memcached Ansible sample **Motivation for the change:** operator-framework/enhancements#47

**Description of the change:** Add Memcached Go sample **Motivation for the change:** operator-framework/enhancements#47

**Description of the change:** Use Memcached sample instead of re-generate the project. **Motivation for the change:** operator-framework/enhancements#47

**Description of the change:** - Use Memcached sample instead of re-generate the project. - Fix/Adjust the e2e tests to work with the sample. **Motivation for the change:** operator-framework/enhancements#47

**Description of the change:** Use Memcached sample instead of re-generate the project. **Motivation for the change:** operator-framework/enhancements#47 Signed-off-by: reinvantveer <rein.van.t.veer@geodan.nl>

**Description of the change:** - Use Memcached sample instead of re-generate the project. - Fix/Adjust the e2e tests to work with the sample. **Motivation for the change:** operator-framework/enhancements#47 Signed-off-by: reinvantveer <rein.van.t.veer@geodan.nl>

camilamacedo86 mentioned this pull request Sep 4, 2020

[Not Merge] - Automate sample generation with helm (POC with samples used in the e2e test to demo the solution proposed in the EP) operator-framework/operator-sdk#3835

Closed

2 tasks

camilamacedo86 requested review from asmacdo, bharathi-tenneti, estroz, jmrodri, joelanford, shawn-hurley, theishshah and varshaprasad96 September 4, 2020 15:21

estroz suggested changes Sep 4, 2020

View reviewed changes

camilamacedo86 requested a review from estroz September 5, 2020 13:20

camilamacedo86 changed the title ~~Proposal for samples generation via GO and in the SDK repo~~ [SDK] - Proposal for samples generation via GO and in the SDK repo Sep 6, 2020

camilamacedo86 changed the title ~~[SDK] - Proposal for samples generation via GO and in the SDK repo~~ WIP : [SDK] - Proposal for samples generation via GO and in the SDK repo Sep 11, 2020

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 11, 2020

camilamacedo86 changed the title ~~WIP : [SDK] - Proposal for samples generation via GO and in the SDK repo~~ [SDK] - Proposal for samples generation via GO and in the SDK repo Sep 15, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 15, 2020

joelanford mentioned this pull request Sep 19, 2020

Replace scorecard images in all files of the tree (As a follow up of #3912 and #3909) operator-framework/operator-sdk#3922

Closed

camilamacedo86 added 2 commits September 24, 2020 14:32

samples proposal sdk

410d645

updates after some suggestions

189733a

varshaprasad96 reviewed Oct 1, 2020

View reviewed changes

asmacdo approved these changes Oct 1, 2020

View reviewed changes

camilamacedo86 and others added 4 commits October 2, 2020 15:14

Update enhancements/samples-generation.md

1ef3f5e

Co-authored-by: Austin Macdonald <austin@redhat.com>

Update enhancements/samples-generation.md

bf98408

Co-authored-by: Austin Macdonald <austin@redhat.com>

Update enhancements/samples-generation.md

3eda037

Co-authored-by: Austin Macdonald <austin@redhat.com>

Update enhancements/samples-generation.md

eea7ead

Co-authored-by: Austin Macdonald <austin@redhat.com>

camilamacedo86 and others added 3 commits October 2, 2020 15:15

Update enhancements/samples-generation.md

1941a2e

Co-authored-by: Austin Macdonald <austin@redhat.com>

Update enhancements/samples-generation.md

ef87d96

Co-authored-by: Austin Macdonald <austin@redhat.com>

Update enhancements/samples-generation.md

3871850

Co-authored-by: Austin Macdonald <austin@redhat.com>

camilamacedo86 requested a review from varshaprasad96 October 2, 2020 14:15

varshaprasad96 approved these changes Oct 2, 2020

View reviewed changes

jmrodri approved these changes Oct 2, 2020

View reviewed changes

openshift-ci-robot assigned jmrodri Oct 2, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 2, 2020

jmrodri merged commit 43d07a4 into operator-framework:master Oct 2, 2020

camilamacedo86 deleted the samples branch October 2, 2020 18:29

This was referenced Oct 6, 2020

feat: add memcached go sample operator-framework/operator-sdk#3980

Merged

feat: add ansible memcached sample operator-framework/operator-sdk#3981

Merged

camilamacedo86 added a commit to operator-framework/operator-sdk that referenced this pull request Oct 7, 2020

feat: add ansible memcached sample (#3981)

8e9811d

**Description of the change:** Add Memcached Ansible sample **Motivation for the change:** operator-framework/enhancements#47

camilamacedo86 added a commit to operator-framework/operator-sdk that referenced this pull request Oct 7, 2020

feat: add memcached go sample (#3980)

44ed2f5

**Description of the change:** Add Memcached Go sample **Motivation for the change:** operator-framework/enhancements#47

camilamacedo86 mentioned this pull request Oct 7, 2020

doc: update places to direct the users to check the samples in /testdata operator-framework/operator-sdk#3995

Merged

2 tasks

This was referenced Oct 16, 2020

feat: use samples in the e2e (helm) operator-framework/operator-sdk#4028

Closed

test-e2e/go : using samples operator-framework/operator-sdk#4056

Merged

camilamacedo86 mentioned this pull request Oct 24, 2020

feat: e2e tests with sample (ansible) operator-framework/operator-sdk#4094

Closed

joelanford mentioned this pull request Oct 29, 2020

test/e2e-helm: use generated helm sample operator-framework/operator-sdk#4134

Merged

camilamacedo86 mentioned this pull request Oct 29, 2020

test-e2e/ansible : using samples operator-framework/operator-sdk#4142

Merged

camilamacedo86 mentioned this pull request Nov 16, 2020

Add bindata for OLM release 0.17.0 operator-framework/operator-sdk#4242

Merged

2 tasks


		## Open Questions

		1. Should not the project has a Memcached GO Sample with webhooks and other without using webhooks since it shows an advanced feature and it used is not mandatory? Note the Go tutorial do not use webhooks.


		### Implementation Details/Notes/Constraints

		- The samples should be build inside of `./testdata/<type>/` path


		#### Generate samples via Go

		- I am a maintainer/contributor, I want to be able to troubleshoot the code used to create the samples, so that I can easily fix the issues.

[SDK] - Proposal for samples generation via GO and in the SDK repo #47

[SDK] - Proposal for samples generation via GO and in the SDK repo #47

Uh oh!

Conversation

camilamacedo86 commented Sep 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

estroz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

camilamacedo86 commented Sep 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

camilamacedo86 commented Sep 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

varshaprasad96 Oct 1, 2020

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Oct 2, 2020

Choose a reason for hiding this comment

Uh oh!

varshaprasad96 Oct 1, 2020

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Oct 2, 2020

Choose a reason for hiding this comment

Uh oh!

jmrodri Oct 2, 2020

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Oct 2, 2020

Choose a reason for hiding this comment

Uh oh!

varshaprasad96 Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asmacdo Oct 1, 2020

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Oct 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

camilamacedo86 commented Oct 2, 2020

Uh oh!

jmrodri left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

camilamacedo86 commented Sep 4, 2020 •

edited

Loading

camilamacedo86 commented Sep 5, 2020 •

edited

Loading

camilamacedo86 commented Sep 5, 2020 •

edited

Loading

varshaprasad96 Oct 1, 2020 •

edited

Loading

camilamacedo86 Oct 2, 2020 •

edited

Loading