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

19373 Update CI for queue services #2424

Merged
merged 26 commits into from
Feb 1, 2024

Conversation

leodube-aot
Copy link
Collaborator

@leodube-aot leodube-aot commented Jan 30, 2024

Issue #: /bcgov/entity#19373

Description of changes:

  • Updated CI for the following queue services:
    • entity-bn
    • entity-emailer
    • entity-filer
    • entity-pay
    • entity-auth
  • Fixed black formatting errors
  • Fixed pylint and flake8 errors

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the lear license (Apache 2.0).

@leodube-aot leodube-aot marked this pull request as draft January 30, 2024 19:05

This comment was marked as resolved.

skips = ["B104"]

[tool.flake8]
ignore = ["F401","E402", "Q000", "E203", "W503"]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have added W503 (line break before binary operator) to the ignore array as we make use of this convention in many places to break up long conditionals. Otherwise, we end up with lines of code that exceed the max-line-length, or we get warning W504 (line break after binary operator) if we split up the lines after the operator.

@leodube-aot
Copy link
Collaborator Author

Marking as ready for review. The testing CI flows are still failing but I think they're just not completely setup yet based on this: https://github.com/bcgov/bcregistry-sre/actions/runs/7733232307

Copy link
Contributor

@tshyun24 tshyun24 left a comment

Choose a reason for hiding this comment

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

Just wondering is it ok to update the config file a lot? I see lots of update from Patrick's ticket for isort and black, should I also update in mine ticket?

@argush3
Copy link
Collaborator

argush3 commented Feb 1, 2024

Just wondering is it ok to update the config file a lot? I see lots of update from Patrick's ticket for isort and black, should I also update in mine ticket?

@tshyun24 I think you should update your PR as per Patrick's example for now. Depending on the BE service we might do specific tweaks.

@leodube-aot
Copy link
Collaborator Author

Just wondering is it ok to update the config file a lot? I see lots of update from Patrick's ticket for isort and black, should I also update in mine ticket?

@tshyun24 I think you should update your PR as per Patrick's example for now. Depending on the BE service we might do specific tweaks.

Just to build off of what Argus said, I basically copied the exact flake8, black, isort and pylint rules from this code snippet in teams. The only change I made was to add W503 to the ignore array in pylint, with my reasoning for doing that in this comment. You'll have to run the black and isort formatters again, then run pylint and flake8 and fix any new linting errors that might pop up (since the rules have now changed for them).

@tshyun24
Copy link
Contributor

tshyun24 commented Feb 1, 2024

Just wondering is it ok to update the config file a lot? I see lots of update from Patrick's ticket for isort and black, should I also update in mine ticket?

@tshyun24 I think you should update your PR as per Patrick's example for now. Depending on the BE service we might do specific tweaks.

Just to build off of what Argus said, I basically copied the exact flake8, black, isort and pylint rules from this code snippet in teams. The only change I made was to add W503 to the ignore array in pylint, with my reasoning for doing that in this comment. You'll have to run the black and isort formatters again, then run pylint and flake8 and fix any new linting errors that might pop up (since the rules have now changed for them).

Thanks Leo! Forgot to refer to the updated pyproject.toml in teams channel

@argush3
Copy link
Collaborator

argush3 commented Feb 1, 2024

Can we update all the pyproject.toml files to include the additions @tshyun24 made in her BE Jobs CI PR.

Specifically, there were two commits, one to add the coverage dependency and one to config the pytest + coverage options.

I think we'll only be able to test Business Pay CI as it's the only one that gets to the Upload coverage to Codecov step in the workflow. But that's fine as we can verify it's working when we create future tickets for fixing the broken units tests.

Copy link

sonarcloud bot commented Feb 1, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Collaborator

@argush3 argush3 left a comment

Choose a reason for hiding this comment

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

Great work!

Thanks for going through the huge number of files!

@leodube-aot leodube-aot merged commit 9971869 into bcgov:feature-legal-name Feb 1, 2024
13 of 18 checks passed
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