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

.github/workflows: don't lint releases #805

Merged
merged 1 commit into from
Nov 20, 2023

Conversation

jecluis
Copy link
Contributor

@jecluis jecluis commented Nov 19, 2023

Linting a release branch makes little sense.

And the action being used in the lint.yaml workflow relies on obtaining a list of changed files since the base commit. This action seems to presume this will be used for pull requests, or tags that are on the same branch as main. Once we do a release branch and a release tag, the action is unable to figure out how to calculate the diff in versions, and we end up with unnecessary failures.

Signed-off-by: Joao Eduardo Luis <[email protected]>

Linting a release branch makes little sense.

And the action being used in the `lint.yaml` workflow relies on
obtaining a list of changed files since the base commit. This action
seems to presume this will be used for pull requests, or tags that are
on the same branch as `main`. Once we do a release branch and a release
tag, the action is unable to figure out how to calculate the diff in
versions, and we end up with unnecessary failures.

Signed-off-by: Joao Eduardo Luis <[email protected]>
@jecluis jecluis added kind/bug Something isn't working area/CI Continuous Integration labels Nov 19, 2023
@jecluis jecluis added this to the v0.23.0 milestone Nov 19, 2023
@jecluis jecluis requested a review from m-ildefons November 19, 2023 23:22
@jecluis jecluis self-assigned this Nov 19, 2023
Copy link
Contributor

@m-ildefons m-ildefons left a comment

Choose a reason for hiding this comment

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

Since there will be release notes and changes to the .readthedocs.yaml and other small things here and there, I think it would be a good thing to keep some of the linters, e.g. codespell, detect-private-key, debug-statements etc. I've never had an issue with this workflow, so I think it's easiest to just keep it as-is. But if you're having issues with a particular linter, we can also split this workflow up and separate that linter out in its own workflow and only run the good ones on the release branches.

@jecluis
Copy link
Contributor Author

jecluis commented Nov 20, 2023

Since there will be release notes and changes to the .readthedocs.yaml and other small things here and there, I think it would be a good thing to keep some of the linters, e.g. codespell, detect-private-key, debug-statements etc.

I find that linting those things outside the scope of the release.yaml a bit silly, because it's not going to prevent us from releasing the artifacts regardless of whether linting fails.

And I don't disagree with checking for those things, in principle, but let's do it where it actually matters, and in a way that actually works.

I've never had an issue with this workflow, so I think it's easiest to just keep it as-is. But if you're having issues with a particular linter, we can also split this workflow up and separate that linter out in its own workflow and only run the good ones on the release branches.

This linter workflow changed a few days ago, it's no longer the same workflow. It relies on an action that fails to resolve the diff in versions when we create the release branch + tag. See failure with debug here: https://github.com/aquarist-labs/s3gw/actions/runs/6921355570/job/18826896212#step:4:481

@irq0
Copy link
Member

irq0 commented Nov 20, 2023

This linter workflow changed a few days ago, it's no longer the same workflow. It relies on an action that fails to resolve the diff in versions when we create the release branch + tag. See failure with debug here: aquarist-labs/s3gw/actions/runs/6921355570/job/18826896212#step:4:481

Unforeseen consequences :(.
I added the changed file step so pre-commit doesn't check the whole repo. This way we lint new instead of all as we did before. We could get rid of the step and revert to the previous behavior. Unfortunately we'd need to fix a couple of shell scripts due to shellcheck now watching

@jecluis
Copy link
Contributor Author

jecluis commented Nov 20, 2023

This way we lint new instead of all as we did before. We could get rid of the step and revert to the previous behavior. Unfortunately we'd need to fix a couple of shell scripts due to shellcheck now watching

And I think that makes sense. I just don't think running this check, standalone, against the release branches/tags make sense 🤷

Copy link
Contributor

@m-ildefons m-ildefons left a comment

Choose a reason for hiding this comment

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

lgtm

@jecluis jecluis merged commit daa6633 into s3gw-tech:main Nov 20, 2023
2 checks passed
@jecluis jecluis deleted the wip-dont-lint-release branch November 20, 2023 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/CI Continuous Integration kind/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants