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

Modified faqs to fix preserveUnknownFields issue from make bundle #5219

Merged
merged 4 commits into from
Sep 22, 2021
Merged

Modified faqs to fix preserveUnknownFields issue from make bundle #5219

merged 4 commits into from
Sep 22, 2021

Conversation

laxmikantbpandhare
Copy link
Member

@laxmikantbpandhare laxmikantbpandhare commented Sep 17, 2021

Description of the change:

Modified faqs to fix preserveUnknownFields issue from make bundle.

Motivation for the change:
preserveUnknownFields missed after make bundle Added script which will add preserveUnknownFields back to the yaml file.

Checklist

If the pull request includes user-facing changes, extra documentation is required:

@laxmikantbpandhare laxmikantbpandhare requested review from jmrodri, fabianvf and varshaprasad96 and removed request for fabianvf and varshaprasad96 September 17, 2021 20:27
@laxmikantbpandhare laxmikantbpandhare self-assigned this Sep 17, 2021
@laxmikantbpandhare laxmikantbpandhare added the kind/documentation Categorizes issue or PR as related to documentation. label Sep 17, 2021
Signed-off-by: laxmikantbpandhare <laxmikantpandhare@gmail.com>
Signed-off-by: laxmikantbpandhare <laxmikantpandhare@gmail.com>
website/content/en/docs/faqs/_index.md Outdated Show resolved Hide resolved
website/content/en/docs/faqs/_index.md Outdated Show resolved Hide resolved
website/content/en/docs/faqs/_index.md Outdated Show resolved Hide resolved
Signed-off-by: laxmikantbpandhare <laxmikantpandhare@gmail.com>
Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @laxmikantbpandhare,

As we spoke, it seems very nice that you could workarround the problem but that does not seem to be the correct approach to solve the problem. Following what (IMHO) we need to do here:

If not:

  • a) we need to clarify why the problems occur, the possible workaround solutions, and that this option is no longer helpful for the next version into the controller-gen and SDK issue
  • b) we can link the issue into the controller-tools repo in the operator-sdk, and we can close that by saying controller-tools and not SDK own the issue
  • c) if that is deprecated in favour of x-kubernetes-preserve-unknown-fields, then we need to describe the version where it was deprecated and, (if possible) when it will be removed/no longer supported. How and when to use x-kubernetes-preserve-unknown-fields (link the k8s docs)
  • d) if it was removed and only valid for v1beta1 CRD/Webhooks and is not a low effort to be solved into the controller tools then it seems a low priority for me.

If yes: then, the fix seems to fit into controller tools and not SDK. We can collaborate with the project to get it done.

However, in POV we cannot start to doc, suggest and officially support workaround scripts to address the problem.

I hope that can help.

Copy link
Member

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 21, 2021
@jmrodri
Copy link
Member

jmrodri commented Sep 21, 2021

Hi @laxmikantbpandhare,

As we spoke, it seems very nice that you could workarround the problem but that does not seem to be the correct approach to solve the problem. Following what (IMHO) we need to do here:

  • Is preserveUnknownFields=false used or not after we got merge the PR: kubernetes-sigs/controller-tools#607? Note that for the next controller-tools/gen release, we will no longer support v1beta1 CRDs and Webhooks

My understanding is that we need that field to be false to support the v1beta1 folks still using SDK.

If not:

  • a) we need to clarify why the problems occur, the possible workaround solutions, and that this option is no longer helpful for the next version into the controller-gen and SDK issue

This is precisely the issue. And in the make bundle it remains all way until we pass the file into the operator-sdk to write it out.

	operator-sdk generate kustomize manifests -q # preserveUnknownFields still exists
	cd config/manager && $(KUSTOMIZE) edit set image controller=$(IMG)  # preserveUnknownFields still exists
	$(KUSTOMIZE) build config/manifests \  # preserveUnknownFields still exists
             | operator-sdk generate bundle -q --overwrite --version $(VERSION) $(BUNDLE_METADATA_OPTS)  # preserveUnknownFields is GONE!

So basically from the user's point of view, operator-sdk REMOVED the field. Underneath the cover it was because of the controller-tools issue you mentioned above, but the users do not care. And because this field is deprecated it doesn't seem worth fixing in the mainline code.

  • b) we can link the issue into the controller-tools repo in the operator-sdk, and we can close that by saying controller-tools and not SDK own the issue

No I don't think we can close the issue saying it's a controller-tools issue. The user sees operator-sdk. So in their eyes operator-sdk was the one that removed it.

  • c) if that is deprecated in favour of x-kubernetes-preserve-unknown-fields, then we need to describe the version where it was deprecated and, (if possible) when it will be removed/no longer supported. How and when to use x-kubernetes-preserve-unknown-fields (link the k8s docs)

If the field should be using this new field, I could be okay with that being documented. But I still think that doesn't really address the users current problem of us removing the field.

  • d) if it was removed and only valid for v1beta1 CRD/Webhooks and is not a low effort to be solved into the controller tools then it seems a low priority for me.

It is low priority. That is why I wanted to simply add the solution to the FAQ so that if someone did run into it, they had a work around.

If yes: then, the fix seems to fit into controller tools and not SDK. We can collaborate with the project to get it done.

However, in POV we cannot start to doc, suggest and officially support workaround scripts to address the problem.

Are we really supporting the workaround script? We are simply offering a possible solution to the problem. I would rather see a possible workaround script that helps a user retain the field.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 22, 2021
Signed-off-by: laxmikantbpandhare <laxmikantpandhare@gmail.com>
Copy link
Member

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 22, 2021
@jmrodri jmrodri merged commit 7bd29b4 into operator-framework:master Sep 22, 2021
@asmacdo asmacdo added this to the v1.13.0 milestone Sep 29, 2021
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. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

preserveUnknownFields is removed from CRDs by operator-sdk generate bundle
5 participants