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

[AMORO-2778][Helm Chart] Add Tests for Helm Chart #2794

Merged
merged 13 commits into from
May 4, 2024

Conversation

lianneli
Copy link
Contributor

@lianneli lianneli commented Apr 28, 2024

Why are the changes needed?

Fix #2778

Brief change log

  • Add Tests for templates in charts/amoro/templates
  • Add unit test usage description to README file
  • Fix file name spelling error: "charts/amoro/templates/amoro-database-secert.yaml" to "charts/amoro/templates/amoro-database-secret.yaml"
  • Fix to delete repeated Label app.kubernetes.io/name: amoro-database-secret in "charts/amoro/templates/amoro-database-secert.yaml"

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

  • Run test locally before making a pull request

image

Documentation

  • Does this pull request introduce a new feature? yes
  • If yes, how is the feature documented? docs(README.md)

@lianneli
Copy link
Contributor Author

@baiyangtx Could you please help to review?

Copy link
Contributor

@baiyangtx baiyangtx left a comment

Choose a reason for hiding this comment

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

This PR is valuable, and now that we already have unittest, why not add it to the workflow? Would you be interested in completing this part in this PR?

@lianneli

Copy link
Contributor

@tcodehuber tcodehuber left a comment

Choose a reason for hiding this comment

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

LGTM, I leave some comments.


Helm Chart for Amoro supports Unit Tests by [helm-unittest](https://github.com/helm-unittest/helm-unittest). Before
submitting PRs, please let all the tests passed .
Copy link
Contributor

Choose a reason for hiding this comment

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

please let all tests passed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

appVersion: 0.1.0
version: 0.6.0
tests:
- it: Amoro database secret should not exist when database type set to derby
Copy link
Contributor

Choose a reason for hiding this comment

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

when database type is set to derby

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

asserts:
- hasDocuments:
count: 0
- it: Amoro database secret should exist when database type set to mysql
Copy link
Contributor

Choose a reason for hiding this comment

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

when database type is set to mysql

Copy link
Contributor

Choose a reason for hiding this comment

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

Some similar contents in this file can be changed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

asserts:
- notExists:
path: metadata.annotations
- it: Amoro Ingress should not contains ingress className if not set
Copy link
Contributor

Choose a reason for hiding this comment

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

Amoro Ingress should not contain ingress className if not set.

Copy link
Contributor

Choose a reason for hiding this comment

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

contains -> contain

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

documentSelector:
path: metadata.name
value: test-table
- it: Amoro Service for table should reflect Node port of set type to NodePort
Copy link
Contributor

Choose a reason for hiding this comment

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

type is set to NodePort

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

@lianneli
Copy link
Contributor Author

This PR is valuable, and now that we already have unittest, why not add it to the workflow? Would you be interested in completing this part in this PR?

@lianneli

Thanks for your advice. I have add a workflow to run unit tests when change helm chart files. Could you help to check if it is proper?

@baiyangtx

.github/workflows/pull-request-on-helmchat.yml Outdated Show resolved Hide resolved
# This workflow will execute Tests when submit a PR.

name: "Pull Request Test For Helm Charts"

Copy link
Contributor

Choose a reason for hiding this comment

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

Add this

concurrency:
  group: ${{ github.workflow }}-${{ github.ref }}
  cancel-in-progress: true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

@lianneli
Copy link
Contributor Author

lianneli commented May 3, 2024

Thanks again. I have accepted all the suggestions. Is there another one?

@baiyangtx

Copy link
Contributor

@baiyangtx baiyangtx left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your contributions.

@baiyangtx baiyangtx merged commit 75ac177 into apache:master May 4, 2024
2 checks passed
@lianneli lianneli deleted the master-fixIssue2778 branch May 5, 2024 04:57
@zhoujinsong zhoujinsong mentioned this pull request Jun 25, 2024
66 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Add Tests for helm chart templates
3 participants