-
Notifications
You must be signed in to change notification settings - Fork 87
Connect chart testing improvements #251
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
base: main
Are you sure you want to change the base?
Conversation
…dispatch action workflow system
edif2008
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functional review: ❓
Code review: ✅
Things look really nice. I like how we've extracted the testing so that we can also run it locally.
Other notes
If i accidentally provide wrong configuration details (e.g. wrong Connect token), I needed to clean the setup before running it again. Might be useful to document this.
| @if [ -z "$(OP_SECRET_VALUE)" ]; then \ | ||
| echo "$(RED)Error: OP_SECRET_VALUE environment variable is required$(NC)"; \ | ||
| echo "$(YELLOW)Set OP_SECRET_VALUE to the expected secret value for testing$(NC)"; \ | ||
| exit 1; \ | ||
| fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit]
This can be extracted into a target that's not exposed. You can pass variable name, expected string name (e.g. "item ID") and the variable you want to verify to it.
| # Colors for output | ||
| RED := \033[0;31m | ||
| GREEN := \033[0;32m | ||
| YELLOW := \033[0;33m | ||
| BLUE := \033[0;34m | ||
| NC := \033[0m # No Color |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[maybe non-blocking]
What if the terminal doesn't support colors? 🤔
This PR improves the e2e tests workflow for Connect chart: