Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Notes on Go tutorial #6314

Closed
dharmit opened this issue Feb 15, 2023 · 6 comments · Fixed by #6384
Closed

Notes on Go tutorial #6314

dharmit opened this issue Feb 15, 2023 · 6 comments · Fixed by #6384
Assignees
Labels
kind/documentation Categorizes issue or PR as related to documentation. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.
Milestone

Comments

@dharmit
Copy link
Contributor

dharmit commented Feb 15, 2023

What is the URL of the document?

https://sdk.operatorframework.io/docs/building-operators/golang/tutorial/

Which section(s) is the issue in?

  1. Define the api section asks to add // +kubebuilder:validation:Maximum=3 marker. This, in turn, causes the kubectl patch command to fail with:
    $ kubectl patch memcached memcached-sample -p '{"spec":{"size": 5}}' --type=merge
    The Memcached "memcached-sample" is invalid: spec.size: Invalid value: 5: spec.size in body should be less than or equal to 3
  2. The default main.go generated using operator-sdk command doesn't have MemcachedReconciler.Recorder initialized like here. It causes the controller to panic in the Reconcile method.
  3. The CR mentioned in Create a Memcached CR section fails to get created due to missing value for containerPort. Adding containerPort: 11211 to the spec helps fix it.

What needs fixing?

  1. Correcting the kubectl patch command or, adding a note in the doc would be helpful.
  2. Mentioning it somewhere in the doc would be helpful, IMO, and save a lot of time for someone trying to get started with Operators for the first time.
  3. Adding containerPort: 11211 to the spec helps fix it.

Additional context

I'm trying the operator-sdk tutorial for the first time, so let me know if I'm making a mistake instead of something being amiss in the docs.

@dharmit dharmit added the kind/documentation Categorizes issue or PR as related to documentation. label Feb 15, 2023
@dharmit
Copy link
Contributor Author

dharmit commented Feb 15, 2023

I can open a PR to address these points I have raised. But I'd like to know from the team if I'm looking at this right or misunderstanding something myself. :)

@jberkhahn jberkhahn self-assigned this Feb 20, 2023
@jberkhahn jberkhahn added this to the v1.29.0 milestone Feb 20, 2023
@jberkhahn
Copy link
Contributor

Sure, we'd love for you to submit a PR to fix these.

  1. Probably just change the max to 5 in the kubebuilder tag.
  2. this probably shouldn't be happening. will have to chat about this and figure out where it needs to be fixed. we'll talk about this at the next triage meeting.
  3. Yeah, add the containerPort: 11211 to the example spec

@jberkhahn jberkhahn removed this from the v1.29.0 milestone Feb 20, 2023
@dharmit dharmit mentioned this issue Feb 21, 2023
2 tasks
@dharmit
Copy link
Contributor Author

dharmit commented Feb 21, 2023

I've opened a PR for 1 & 3.

@varshaprasad96
Copy link
Member

For (2), I see what the issue is: The operator-idk init command does not initialize the recorder used for recording events from EventSource. Whereas in the reconciler code, we record a delete event, which when directly copied by users into the project scaffolded by operator-sdk init command fails. The two options for solving this (imo) could be:

  1. Add a TODO section in the sample - main.go, explaining that the user needs to initialize the recorder as it is one of the best practices for recording events. Add documentation regarding the same in tutorial.
  2. Use deploy-image plugin by default to scaffold out operator-sdk. We are having the testdata scaffolded out using deploy-image plugin to help users follow the best practices but the sdk init command by default doesn't use it in tutorial.

I would vote for (1), given:

  • we would not want a first-time user of Operator SDK to use a complicated command explaining all the flags while getting started.
  • It is simpler than making deploy-image plugin default, especially since the plugin is in alpha stage - not yet stable.

@varshaprasad96 varshaprasad96 added this to the v1.29.0 milestone Feb 27, 2023
@dharmit
Copy link
Contributor Author

dharmit commented Feb 28, 2023

Add a TODO section in the sample - main.go, explaining that the user needs to initialize the recorder as it is one of the best practices for recording events.

When/where is this sample main.go used/referenced? I'm asking because I didn't come across it while going through the Go tutorial. Which makes me wonder as to how the user will notice this TODO.

Or, would adding it to this place automatically ensure such a TODO shows up in the main.go generated by operator-sdk init? I don't think so, but then I don't know how operator-sdk init works.

Add documentation regarding the same in tutorial.

+1. That would surely help.

@openshift-bot
Copy link

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/documentation Categorizes issue or PR as related to documentation. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants