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

Add very useful values common with the main Cert-manager chart #96

Merged
merged 1 commit into from
Aug 1, 2023
Merged

Add very useful values common with the main Cert-manager chart #96

merged 1 commit into from
Aug 1, 2023

Conversation

MaesterZ
Copy link
Contributor

@MaesterZ MaesterZ commented Dec 16, 2022

New values like in https://github.com/cert-manager/cert-manager/blob/master/deploy/charts/cert-manager/values.yaml

  • commonLabels
  • deploymentAnnotations
  • podAnnotations
  • podLabels
  • nodeSelector
  • affinity
  • tolerations
  • priorityClassName

@jetstack-bot jetstack-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 16, 2022
@MaesterZ MaesterZ marked this pull request as ready for review December 16, 2022 15:16
@jetstack-bot jetstack-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 16, 2022
@MaesterZ
Copy link
Contributor Author

Should we bump the chart API version?

[ERROR] Chart.yaml: chart type is not valid in apiVersion 'v1'. It is valid in apiVersion 'v2'

@mgoodness
Copy link

mgoodness commented Feb 13, 2023

Should we bump the chart API version?

[ERROR] Chart.yaml: chart type is not valid in apiVersion 'v1'. It is valid in apiVersion 'v2'

Yes. Helm v2 (apiVersion: v1) was deprecated in November 2020.

@MaesterZ
Copy link
Contributor Author

Done

@irbekrm
Copy link
Contributor

irbekrm commented Feb 20, 2023

/ok-to-test

@MaesterZ
Copy link
Contributor Author

MaesterZ commented Feb 21, 2023

Not sure why exactly the CI is failing @irbekrm, not related to my changes:

Error: google-github-actions/auth failed with: retry function failed after 1 attempt: gitHub Actions did not inject $ACTIONS_ID_TOKEN_REQUEST_TOKEN or $ACTIONS_ID_TOKEN_REQUEST_URL into this job. This most likely means the GitHub Actions workflow permissions are incorrect, or this job is being run from a fork. For more information, please see https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token

@MaesterZ MaesterZ mentioned this pull request Feb 21, 2023
@MaesterZ
Copy link
Contributor Author

Is anyone available to review this and fix the pipeline? 🙏🏻

@irbekrm
Copy link
Contributor

irbekrm commented Mar 21, 2023

@MaesterZ I will take a look at what we can do to allow contributions, hopefully at some point this week.

Thank you for your PR.

@inteon inteon closed this Apr 5, 2023
@inteon inteon reopened this Apr 5, 2023
@inteon inteon closed this Apr 5, 2023
@inteon inteon reopened this Apr 5, 2023
@MaesterZ
Copy link
Contributor Author

MaesterZ commented Apr 17, 2023

Do I have anything to fix here? It's a google cloud authentication error.

@inteon
Copy link
Member

inteon commented Apr 22, 2023

Do I have anything to fix here? It's a google cloud authentication error.

That is an old test, the required tests are passing. We just need to review.

@ghost
Copy link

ghost commented Aug 1, 2023

Still looking to get this merge, not sure how can I help. We can also bump to 0.7.0 in a new helm chart release maybe.

Copy link
Member

@SgtCoDFish SgtCoDFish left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

Thanks for this! ❤️

Sorry it's taken a while to get this reviewed and merged. I'll try to take a look into releasing a new version of this issuer either today or tomorrow so that these changes are released live 👍

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 1, 2023
@SgtCoDFish SgtCoDFish merged commit 5dd5ef8 into cert-manager:main Aug 1, 2023
@ghost
Copy link

ghost commented Aug 1, 2023

Sorry it's taken a while to get this reviewed and merged. I'll try to take a look into releasing a new version of this issuer either today or tomorrow so that these changes are released live +1

Thanks!

@wallrj
Copy link
Member

wallrj commented Mar 13, 2024

@MaesterZ I've been writing some documentation explaining why to set priorityClassName when deploying cert-manager and what the value should be. Please read and feedback if you have time:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants