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

feat: add helm charts #1

Merged
merged 8 commits into from
Jun 17, 2024
Merged

feat: add helm charts #1

merged 8 commits into from
Jun 17, 2024

Conversation

smuu
Copy link
Member

@smuu smuu commented Jun 12, 2024

Overview

Copy link
Contributor

@tty47 tty47 left a comment

Choose a reason for hiding this comment

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

🚀

Signed-off-by: Smuu <[email protected]>
Comment on lines 27 to 33
- name: Run chart-testing (list-changed)
id: list-changed
run: |
changed=$(ct list-changed --target-branch ${{ github.event.repository.default_branch }})
if [[ -n "$changed" ]]; then
echo "changed=true" >> "$GITHUB_OUTPUT"
fi
Copy link
Member

Choose a reason for hiding this comment

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

unless these tests take a long time to run, I would remove this and just have them always run.

One benefit of them always running is if there is a dependency update, that doesn't change any helm chart files, there might be an issue that gets missed because the tests didn't run.

Comment on lines +11 to +13
| Name | Email | Url |
| ---- | ------ | --- |
| Celestia Labs | | <https://github.com/celestiaorg/celestia-helm-charts> |
Copy link
Member

Choose a reason for hiding this comment

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

why not just delete the email column?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is auto-generated helm docs

Copy link
Member

Choose a reason for hiding this comment

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

nit: might as well spell out the name for the filename.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the common term in helm charts for that resource

Comment on lines +11 to +13
| Name | Email | Url |
| ---- | ------ | --- |
| Celestia Labs | | <https://github.com/celestiaorg/celestia-helm-charts> |
Copy link
Member

Choose a reason for hiding this comment

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

same

Copy link
Member Author

Choose a reason for hiding this comment

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

This is auto-generated helm docs

Copy link
Member

Choose a reason for hiding this comment

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

same

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the common term in helm charts for that resource

Copy link
Member

@MSevey MSevey left a comment

Choose a reason for hiding this comment

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

utack

Some future considerations:

  • Add yaml linter
  • Add toml linter
  • add tests for the scripts

@smuu smuu merged commit ac54d08 into main Jun 17, 2024
1 of 2 checks passed
@smuu smuu deleted the smuu/add-charts branch June 17, 2024 07:09
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.

3 participants