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 repo setup 2 #6

Closed
wants to merge 42 commits into from
Closed

Add repo setup 2 #6

wants to merge 42 commits into from

Conversation

ElePT
Copy link
Collaborator

@ElePT ElePT commented Jul 19, 2023

Re-opening PR #1 from branch in upstream repo to attempt to trigger actions.

Comment on lines 76 to 83
- name: Copyright Check
run: |
python tools/check_copyright.py -check
if: ${{ !cancelled() }}
shell: bash
- run: make spell
if: ${{ !cancelled() }}
shell: bash
Copy link
Collaborator Author

@ElePT ElePT Jul 19, 2023

Choose a reason for hiding this comment

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

I have removed the spelling and copyright check as a temporary measure, as these did not exist in terra and logically fail at the moment. I think that a spell check can be pretty helpful, I am not discussing that, but I would prefer to start with a minimal CI and then, if we want to add these back, we can open a PR that:

  1. adds the checks back
  2. fixes the code to pass the checks

(for this reason I have only deleted them from main.yml but keep the related tools that can be ran locally).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Similarly, I have removed the deprecation-related actions.

Copy link
Member

@woodsp-ibm woodsp-ibm Jul 19, 2023

Choose a reason for hiding this comment

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

The spelling, copyright, mypy etc all passed when this was in Aqua and was run under the sort of CI in the apps. The combined deprecation is something that we added - as otherwise going over all the logs was tedious. The CI was setup to be like Terra but we added spelling, copyright checking and mypy since we, most likely driven by me, thought them important. Since then Terra now errors on deprecation whereas we chose to gather the info so that a dev could/should fix the warnings but did not fail CI. That could be revisited but is likely to add more hard failures to CI.

My take would be to have things commensurate with apps, since we are the team that is presently dealing with all this so its all self-consistent across these repos and then go from there. I understand getting things working incrementally here though as its harder to go from nothing to everything in one go!

On the pyproject file - if we had had to move the lint stuff into it now, which me I would have considered that for later, I would have just done that based on the file we had already, As such now this one from Terra has wheel based stuff we are not using, stuff for ruff (which maybe we would want to run and transition over too as well). In the end the CI ends up different - we have other stuff for publishing the docs on the fly as it has to be careful around release notes as we tend to push over PRs to stable ahead of any bug fix release and nothing from these should appear if we do a docs updated until they are tagged into a release. Ideally it might all end up the same and consistent - but with this all moving to the community, with different focus/team etc keeping things the same is always a constant activity. We tried over the years to do things similarly and mirror things terra did even as we did other stuff too. Like the move to have the lint stuff in pyproject file which terra did - the pylint config was from terra to have things consistent - we just did not get around to doing that in the apps yet,

@ElePT ElePT closed this Jul 20, 2023
@ElePT ElePT deleted the add-repo-setup branch July 20, 2023 13:56
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.

2 participants