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 manual e2e testing procedure #363

Merged
merged 9 commits into from
Sep 24, 2024
Merged

Conversation

QuantumEnigmaa
Copy link
Contributor

@QuantumEnigmaa QuantumEnigmaa self-assigned this Sep 10, 2024
@QuantumEnigmaa QuantumEnigmaa marked this pull request as ready for review September 11, 2024 07:44
@QuantumEnigmaa QuantumEnigmaa requested a review from a team as a code owner September 11, 2024 07:44
Copy link
Contributor

@QuentinBisson QuentinBisson left a comment

Choose a reason for hiding this comment

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

I do not really sée hère how to run thé testing procedure at all right?
And could we have thé requirements (Loki canary enabled, check app is in déployed state in a script right?

Co-authored-by: Quentin Bisson <quentin@giantswarm.io>
@QuantumEnigmaa
Copy link
Contributor Author

could we have thé requirements (Loki canary enabled, check app is in déployed state in a script right?

Sure I can write something for that :)

@QuantumEnigmaa
Copy link
Contributor Author

@QuentinBisson I added a small script for checking essential requirements (app is deployed and loki-canary enabled)

Copy link
Contributor

@QuentinBisson QuentinBisson left a comment

Choose a reason for hiding this comment

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

LGTM but I'm no bash experts

@QuantumEnigmaa
Copy link
Contributor Author

I tested it and it works :)

@QuantumEnigmaa
Copy link
Contributor Author

I also added a PR message template so that the user is invited to follow the testing procedure before asking for reviews.


deployed=$(kubectl get app -n giantswarm loki -o yaml | yq .status.release.status)

[[ "$deployed" != "deployed" ]] && exit_error "loki app is not in deployed state. Please fix the app before retrying" || echo "loki app is indeed in deployed state"
Copy link
Contributor

Choose a reason for hiding this comment

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

The || is useless: if the test matches, it will exit_error.
Also, most linters don't like too long lines.

So I suggest something like this:

Suggested change
[[ "$deployed" != "deployed" ]] && exit_error "loki app is not in deployed state. Please fix the app before retrying" || echo "loki app is indeed in deployed state"
[[ "$deployed" != "deployed" ]] \
&& exit_error "loki app is not in deployed state. Please fix the app before retrying"
echo "✔️ loki app is in deployed state"

Which would apply to all tests then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. As for the || this is just to keep logging updates on the advancement of the process. But if you think this isn't needed, I can get rid of those as well

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't say the log message informing that the test passed successfully is useless.
I said the condition before it is useless.
My suggestion keeps the message, but removes the condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh ok I get it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hervenicol I added back the messages without the condition

@QuantumEnigmaa
Copy link
Contributor Author

@hervenicol asking for your review again :)

Copy link
Contributor

@hervenicol hervenicol left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 11 to 13
deployed=$(kubectl get app -n giantswarm loki -o yaml | yq .status.release.status)

[[ "$deployed" != "deployed" ]] \
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
deployed=$(kubectl get app -n giantswarm loki -o yaml | yq .status.release.status)
[[ "$deployed" != "deployed" ]] \
status=$(kubectl get app -n giantswarm loki -o yaml | yq .status.release.status)
[[ "$status" != "deployed" ]] \

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used appStatus to be even more precise :)

@QuantumEnigmaa QuantumEnigmaa merged commit 9fe200b into main Sep 24, 2024
6 checks passed
@QuantumEnigmaa QuantumEnigmaa deleted the add-manual-e2e-testing branch September 24, 2024 06:34
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