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

Linter CI part 1 #52

Open
wants to merge 32 commits into
base: main
Choose a base branch
from
Open

Linter CI part 1 #52

wants to merge 32 commits into from

Conversation

yaswant
Copy link

@yaswant yaswant commented Jan 13, 2025

Description

Summary

Code linting via Super-Linter GitHub Action

Changes

Added

  • Linter workflow file workflow/lint.yml
  • Some manual language-specific overrides in linters/
  • CODEOWNERS, currently @ssdteam but we can revisit and include individuals for each component in a separate PR
  • A PR template

Dependency

None.

Impact

Automatic checks to assist code review

Issues addressed

Resolves

#51

Coordinated merge

NA

Checklist

  • I have performed a self-review of my own changes

@yaswant yaswant self-assigned this Jan 13, 2025
@yaswant yaswant linked an issue Jan 13, 2025 that may be closed by this pull request
Copy link
Author

@yaswant yaswant left a comment

Choose a reason for hiding this comment

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

some minor changes in ruff.toml

.github/linters/.ruff.toml Outdated Show resolved Hide resolved
.github/linters/.ruff.toml Outdated Show resolved Hide resolved
.github/linters/.ruff.toml Outdated Show resolved Hide resolved
.github/linters/.ruff.toml Outdated Show resolved Hide resolved
@yaswant
Copy link
Author

yaswant commented Jan 24, 2025

@yaswant
Copy link
Author

yaswant commented Jan 25, 2025

@james-bruten-mo Don't think there is anyway to override black defaults for SuperLinter. I see now SciTools/iris switched from black to ruff. So there must be a way AVD use the tool. I take the message that if you prefer black, then just don't try to override defaults via command-line.

@yaswant yaswant requested review from r-sharp and james-bruten-mo and removed request for mo-nikosbaltas January 27, 2025 13:19
@yaswant
Copy link
Author

yaswant commented Jan 27, 2025

This is now ready for review and merge. @Pierre-siddall you can step away from the PR for now, but FYI we have disabled ruff in the workflow which will be revisited in later.

Requesting reviews from:

  • @james-bruten-mo to check implementation changes
  • @r-sharp to check POSIX compliance in the script we discussed in teams chat
  • @t00sa workflow implantation
    Thanks

Copy link
Contributor

@james-bruten-mo james-bruten-mo left a comment

Choose a reason for hiding this comment

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

I've just used this branch for the kgo install I've been doing today. This highlighted a couple of issues:

  • The xc40's require scitools in order to have python3 available - this causes f-string issues when running kgo_update.py - I think we can just add module load scitools to the command in the new function as there's no problem with having it loaded on other platforms.
  • The new printf syntax doesn't colour the strings correctly - instead it just prints out the colour code string. I tested Sams fix from Fix UMDP3 checker linting issues #53 which is to use echo -ne instead and that seems to work so suggest we go with that? I highlighted the locations in meto_run_kgo_script.sh but not in meto_update_kgo.sh.

Cheers

kgo_updates/kgo_update/meto_run_kgo_script.sh Outdated Show resolved Hide resolved
kgo_updates/kgo_update/meto_run_kgo_script.sh Show resolved Hide resolved
kgo_updates/kgo_update/meto_run_kgo_script.sh Outdated Show resolved Hide resolved
kgo_updates/kgo_update/meto_run_kgo_script.sh Outdated Show resolved Hide resolved
kgo_updates/kgo_update/meto_run_kgo_script.sh Outdated Show resolved Hide resolved
@Pierre-siddall
Copy link

This is now ready for review and merge. @Pierre-siddall you can step away from the PR for now, but FYI we have disabled ruff in the workflow which will be revisited in later.

Requesting reviews from:

  • @james-bruten-mo to check implementation changes
  • @r-sharp to check POSIX compliance in the script we discussed in teams chat
  • @t00sa workflow implantation
    Thanks

@yaswant gotcha I'll step back from checking this out and focus on other stuff.

@james-bruten-mo
Copy link
Contributor

I've had another go at installing kgo with this branch and hit issues again.
Given those changes are becoming reasonably large, can I suggest that we split a refactor of the the kgo shell script off into a separate PR, and just enforce shellcheck compliance (probably with some exemptions) in this ticket? Good development practice would probably suggest this ticket should only be for achieving linter compliance anyway.
We can then test the install script changes more thoroughly in slower time.

@yaswant
Copy link
Author

yaswant commented Jan 30, 2025

I've had another go at installing kgo with this branch and hit issues again. Given those changes are becoming reasonably large, can I suggest that we split a refactor of the the kgo shell script off into a separate PR, and just enforce shellcheck compliance (probably with some exemptions) in this ticket? Good development practice would probably suggest this ticket should only be for achieving linter compliance anyway. We can then test the install script changes more thoroughly in slower time.

I've reverted the script back to the way it was before with fixes against shellcheck. Please give it another go.

@james-bruten-mo
Copy link
Contributor

I've tested again and the printf statements with the colour codes at the end don't colour the line correctly. I've reverted these and added the relevant shellcheck ignores if that's an acceptable way to go?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include code linting workflow
3 participants