Skip to content

Conversation

estroz
Copy link
Member

@estroz estroz commented Mar 9, 2021

Description of the change:

  • internal/plugins/manifests/v2: initialize project with a config/manifests/kustomization.yaml that contains a patch to remove the cert volume and mount from the default manager configuration.
  • internal/cmd/operator-sdk/generate/kustomize: use the manifests/v2 kustomization.yaml for backwards-compatibility.

Motivation for the change: no kustomize helpers are created to enable webhooks by default. This is considered a bug since the CSV generator supports webhooks (see #4244).

Closes #4240
Closes #4244
Closes #4439

/kind bug

Checklist

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

@openshift-ci-robot openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Mar 9, 2021
@estroz estroz temporarily deployed to deploy March 9, 2021 22:22 Inactive
@estroz estroz temporarily deployed to deploy March 9, 2021 22:22 Inactive
@estroz estroz temporarily deployed to deploy March 9, 2021 22:22 Inactive
@estroz estroz temporarily deployed to deploy March 9, 2021 22:22 Inactive
@estroz estroz temporarily deployed to deploy March 9, 2021 22:22 Inactive
@estroz estroz temporarily deployed to deploy March 9, 2021 22:22 Inactive
@estroz
Copy link
Member Author

estroz commented Mar 9, 2021

Still need to update docs and add a changelog

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.

IMO we are just missing some nits to get this merged:

  • We need to provide the changelog which also will be useful for those who face this problem.
  • Would be nice to add the manual steps to fix that in the FAQ as well. Do we have them already?
@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 11, 2021
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 16, 2021
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.

/lgtm
/approved

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 16, 2021
@camilamacedo86
Copy link
Contributor

/hold

It is true. It is missing the changelog and docs.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 16, 2021
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-robot
Copy link

New changes are detected. LGTM label has been removed.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 19, 2021
@estroz
Copy link
Member Author

estroz commented Mar 19, 2021

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 19, 2021
@estroz estroz temporarily deployed to deploy March 19, 2021 20:16 Inactive
@estroz estroz temporarily deployed to deploy March 19, 2021 20:16 Inactive
@estroz estroz temporarily deployed to deploy March 19, 2021 20:16 Inactive
@estroz estroz temporarily deployed to deploy March 19, 2021 20:16 Inactive
@estroz estroz temporarily deployed to deploy March 19, 2021 20:16 Inactive
@estroz estroz temporarily deployed to deploy March 19, 2021 20:16 Inactive
@estroz estroz force-pushed the bugfix/csv-webhooks-4244 branch from 9adf23f to bbfcb01 Compare March 19, 2021 20:55
@estroz estroz temporarily deployed to deploy March 19, 2021 20:55 Inactive
@estroz estroz temporarily deployed to deploy March 19, 2021 20:55 Inactive
@estroz estroz temporarily deployed to deploy March 19, 2021 20:55 Inactive
@estroz estroz temporarily deployed to deploy March 19, 2021 20:55 Inactive
@estroz estroz temporarily deployed to deploy March 19, 2021 20:55 Inactive
@estroz estroz temporarily deployed to deploy March 19, 2021 20:55 Inactive
Signed-off-by: Eric Stroczynski <ericstroczynski@gmail.com>
@estroz estroz force-pushed the bugfix/csv-webhooks-4244 branch from bbfcb01 to a9048aa Compare March 19, 2021 21:06
@estroz estroz temporarily deployed to deploy March 19, 2021 21:07 Inactive
@estroz estroz temporarily deployed to deploy March 19, 2021 21:07 Inactive
@estroz estroz temporarily deployed to deploy March 19, 2021 21:07 Inactive
@estroz estroz temporarily deployed to deploy March 19, 2021 21:07 Inactive
@estroz estroz temporarily deployed to deploy March 19, 2021 21:07 Inactive
@estroz estroz temporarily deployed to deploy March 19, 2021 21:07 Inactive
@estroz estroz changed the title (manifests/v2) add support for webhooks by default, with minimal changes (manifests/v2) add support for webhooks by default and docs Mar 19, 2021
@estroz estroz merged commit 163c657 into operator-framework:master Mar 19, 2021
@estroz estroz deleted the bugfix/csv-webhooks-4244 branch March 19, 2021 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
5 participants