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(chaos): Bump duration of experiments #29

Merged
merged 46 commits into from
Feb 2, 2023

Conversation

merkata
Copy link
Contributor

@merkata merkata commented Jan 10, 2023

This PR addresses a timeout in the chaos step of the CI/CD pipeline where the discourse charm does not become healthy in the specified duration (default of 60 seconds). A local test confirms the charm becomes stable, so with this change, the total duration of the chaos experiments is increased, so that the test can (hopefully) pass.

With fixing the pipeline run, some additional steps were needed:

  • there is now a cleanup of S3 related variables in the integration test for S3
  • I had to add dependencies for the linting to pass
  • Functions in the unit and integration tests are refactored in order for pylint to pass
  • Some pylint issues are disabled, please have a look and supply feedback
  • I added a trivy.yaml config fail to ensure trivy doesn't break the pipeline and created an issue to address trivy findings
  • I added a workflow action to sync Github issues to our Jira (so that the issue above lands in Jira as well)

NOTE I've cherry picked commits based on scope and have created additional PRs for Jira, Trivy and linting, so once these are merged, this PR will be slimmer and will have only Chaos test related changes. Respective issues (linting, trivy) are fixed in the respective PRs. Thanks!

@merkata merkata requested a review from a team as a code owner January 10, 2023 09:38
arturo-seijas
arturo-seijas previously approved these changes Jan 10, 2023
Copy link
Collaborator

@arturo-seijas arturo-seijas left a comment

Choose a reason for hiding this comment

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

LGTM

arturo-seijas
arturo-seijas previously approved these changes Jan 10, 2023
Copy link
Collaborator

@arturo-seijas arturo-seijas left a comment

Choose a reason for hiding this comment

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

LGTM

amandahla
amandahla previously approved these changes Jan 10, 2023
@merkata merkata dismissed stale reviews from amandahla and arturo-seijas via 4c6f101 January 12, 2023 15:24
arturo-seijas
arturo-seijas previously approved these changes Jan 12, 2023
Copy link
Collaborator

@arturo-seijas arturo-seijas left a comment

Choose a reason for hiding this comment

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

LGTM

amandahla
amandahla previously approved these changes Jan 13, 2023
weiiwang01
weiiwang01 previously approved these changes Jan 13, 2023
jdkandersson
jdkandersson previously approved these changes Jan 15, 2023
arturo-seijas
arturo-seijas previously approved these changes Jan 16, 2023
Copy link
Collaborator

@arturo-seijas arturo-seijas left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@gregory-schiano gregory-schiano left a comment

Choose a reason for hiding this comment

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

LGTM

tests/unit/test_charm.py Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
trivy.yaml Outdated Show resolved Hide resolved
@merkata merkata marked this pull request as ready for review January 30, 2023 09:25
@gregory-schiano
Copy link
Contributor

I'm not a big fan of leaving so many disabled line-too-long, wasn't there a better approach for these ?

@merkata
Copy link
Contributor Author

merkata commented Jan 30, 2023

I'm not a big fan of leaving so many disabled line-too-long, wasn't there a better approach for these ?

I did try finding a middle ground between black and pylint, did not succeed. To me it is trial and error - bend a line somewhere, see whether black bends it elsewhere, watch pylint complain again. There are 8 ignores, I'd leave this as an issue in the backlog with a good-first-issue label for someone to tackle. I'm too old for this.

Copy link
Contributor

@jdkandersson jdkandersson left a comment

Choose a reason for hiding this comment

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

This looks weirdly similar to another PR: #33

.github/workflows/integration_test.yaml Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
@merkata
Copy link
Contributor Author

merkata commented Jan 31, 2023

This looks weirdly similar to another PR: #33

Yes, they are indeed similar - this PR here is the base one and I split it in 4 different PRs with cherry picks - one about Jira sync, one about Trivy config, one about linter issues and that one here related to the Chaos tests. This would be the last PR to merge and with syncing from main, only the chaos related changes will be here.

@merkata
Copy link
Contributor Author

merkata commented Jan 31, 2023

I'm not a big fan of leaving so many disabled line-too-long, wasn't there a better approach for these ?

Well @jdkandersson is also not a big fan of line-too-long and left a bunch of great feedback on the feat/linter PR, so if you have a look at that, you'll see there are no more line-too-long disables :) Thanks for commenting!

@github-actions
Copy link
Contributor

github-actions bot commented Feb 2, 2023

Test coverage for 922eb89

Name           Stmts   Miss Branch BrPart  Cover   Missing
----------------------------------------------------------
src/charm.py     180     21     56      4    88%   279->283, 376-377, 404-406, 428-432, 441-452, 463->exit, 478-480
----------------------------------------------------------
TOTAL            180     21     56      4    88%

Static code analysis report

Run started:2023-02-02 13:41:29.563146

Test results:
  No issues identified.

Code scanned:
  Total lines of code: 1196
  Total lines skipped (#nosec): 4
  Total potential issues skipped due to specifically being disabled (e.g., #nosec BXXX): 0

Run metrics:
  Total issues (by severity):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
  Total issues (by confidence):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
Files skipped (0):

Copy link
Contributor

@gtrkiller gtrkiller left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@amandahla amandahla left a comment

Choose a reason for hiding this comment

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

LGTM

@merkata merkata merged commit bb43cd5 into main Feb 2, 2023
@merkata merkata deleted the feat/configurable-chaos-duration branch February 2, 2023 18:33
@jdkandersson
Copy link
Contributor

58 comments and 46 commits later 😉
Screenshot 2023-02-03 at 4 15 45 pm

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.

None yet

8 participants