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

Tolerations option #47

Merged
merged 1 commit into from
Jan 18, 2023
Merged

Tolerations option #47

merged 1 commit into from
Jan 18, 2023

Conversation

chrisaq
Copy link
Contributor

@chrisaq chrisaq commented Jan 9, 2023

Description of the change

Add optional tolerations to the chart.

Existing or Associated Issue(s)

Additional Information

Not much to add here, it lets you choose which nodes in the cluster the containers are deployed to.

This is required in some cluster configurations.

Checklist

  • Chart version bumped in Chart.yaml according to semver.
  • Variables are documented in the values.yaml and added to the README.md. The helm-docs utility can be used to generate the necessary content. Use helm-docs --dry-run to preview the content.
  • JSON Schema generated.
  • List tests pass for Chart using the Chart Testing tool and the ct lint command.

@chrisaq chrisaq requested a review from a team as a code owner January 9, 2023 11:30
charts/backstage/README.md Outdated Show resolved Hide resolved
@chrisaq chrisaq requested a review from tumido January 10, 2023 09:14
charts/backstage/values.yaml Outdated Show resolved Hide resolved
Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@chrisaq can you please rebase this and bump the chart version once more?

@chrisaq chrisaq force-pushed the tolerations_option branch from f19f2f7 to d5b9eaa Compare January 17, 2023 12:40
@chrisaq
Copy link
Contributor Author

chrisaq commented Jan 17, 2023

@tumido Is it ok now, or do you have any other suggestions?

@tumido
Copy link
Member

tumido commented Jan 17, 2023

@chrisaq nope, your code looks good to me. 🙂 Now we just need it to pass CI, please check on the README.md and either run pre commit or see the diff at https://github.com/backstage/charts/actions/runs/3939323538/jobs/6739330501 . 🙂

@chrisaq
Copy link
Contributor Author

chrisaq commented Jan 17, 2023

@chrisaq nope, your code looks good to me. slightly_smiling_face Now we just need it to pass CI, please check on the README.md and either run pre commit or see the diff at https://github.com/backstage/charts/actions/runs/3939323538/jobs/6739330501 . slightly_smiling_face

@tumido Thanks, I left in a double entry in the README which I removed, so I believe the CI should pass now.

@chrisaq
Copy link
Contributor Author

chrisaq commented Jan 17, 2023

@tumido Sorry to bother you again.
It seems to fail on something generated from the updated semver, but it is not obvious to me why changing the number from 13 to 14 would trigger this.
Any ideas?

@tumido
Copy link
Member

tumido commented Jan 17, 2023

@chrisaq no worries. 🙂

Your PR is changing the chart version to 0.14.0 https://github.com/backstage/charts/pull/47/files#diff-8316e61c38ae132c31764bc73c703fefb6bfe57770e77aa97e63c0f9f490a1fcR18 . This change needs to be reflected in the README.md as well:

-![Version: 0.13.0](https://img.shields.io/badge/Version-0.13.0-informational?style=flat-square) ![Type: application](https://img.shields.io/badge/Type-application-informational?style=flat-square)
+![Version: 0.14.0](https://img.shields.io/badge/Version-0.14.0-informational?style=flat-square) ![Type: application](https://img.shields.io/badge/Type-application-informational?style=flat-square)

We want the README to display current data, not the previous version 🙂

If you want, you can get the README.md re-generated on the fly before each commit - you can do so by installing pre-commit as a git hook (via pre-commit install).

@chrisaq chrisaq force-pushed the tolerations_option branch from c338226 to 810f2dc Compare January 17, 2023 14:11
Copy link
Member

@tumido tumido 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

@chrisaq chrisaq force-pushed the tolerations_option branch from 810f2dc to 900cb62 Compare January 17, 2023 14:21
@chrisaq
Copy link
Contributor Author

chrisaq commented Jan 17, 2023

Just cleaned up the commit log, thanks for the pre-commit tip.

@tumido
Copy link
Member

tumido commented Jan 17, 2023

@chrisaq sigh.. there seems to be one last issue. 😕 It won't allow me to merge your PR unless your commit is signed.

image

https://docs.github.com/articles/about-gpg/

Can you please look into that?

@chrisaq chrisaq force-pushed the tolerations_option branch from 900cb62 to 7ff8407 Compare January 18, 2023 09:38
@chrisaq
Copy link
Contributor Author

chrisaq commented Jan 18, 2023

@tumido Thank you for your continued patience.
I did a commit with -s and now it says it's signed-off by me, does it match the requirements?
-- nevermind, I see it's still marked as blocked below, I'll try to figure it out.

@chrisaq chrisaq force-pushed the tolerations_option branch from 7ff8407 to 0951406 Compare January 18, 2023 12:29
@chrisaq
Copy link
Contributor Author

chrisaq commented Jan 18, 2023

@tumido seems green here now!

@tumido tumido merged commit 52a9c3a into backstage:main Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants