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

fix(ci): lint issue on update-monorepo-lockfiles.yml #26920

Merged
merged 1 commit into from
Jan 31, 2024

Conversation

dpgaspar
Copy link
Member

@dpgaspar dpgaspar commented Jan 31, 2024

SUMMARY

Fixes lint issue on update-monorepo-lockfiles.yml, impacts open PRs

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@michael-s-molina
Copy link
Member

@dpgaspar All other workflow files have an empty line at the end. The formatter also adds an empty line automatically.

@michael-s-molina
Copy link
Member

@dpgaspar All other workflow files have an empty line at the end. The formatter also adds an empty line automatically.

It looks like committed files don't have that line. LGTM.

@michael-s-molina michael-s-molina merged commit d8f7e2c into apache:master Jan 31, 2024
31 checks passed
@dpgaspar dpgaspar deleted the fix/ci-lint-issue branch January 31, 2024 14:22
@rusackas
Copy link
Member

Ugh... not sure how this snuck through, but this shouldn't have been mergeable in the first place, right?

@mistercrunch
Copy link
Member

Do we not re-run pre-commit on CI?

@mistercrunch
Copy link
Member

Oh @rusackas it's because of this -> https://github.com/apache/superset/blob/master/.github/workflows/superset-python-misc.yml#L10-L13, maybe we should run pre-commit across the board, it's pretty cheap anyways, and many checks are not python-specific anymore

mistercrunch added a commit that referenced this pull request Jan 31, 2024
#26920 caught an issue that
should have been picked up by pre-commit (by the user), but then
re-caught by CI, preventing the merge.

The reason is was not caught was that the CI check is part of a group of
python misc checks that trigger only where the python package's folder
has changed. In reality, the python utility pre-commit we use here is
used for much more than just Python files, so we should run it across
the repo.

Here I simply factored out the check into its own GitHub Action that
triggers for every single CI run.
@mistercrunch
Copy link
Member

Preventing this here -> #26942

sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 2024
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 4.0.0 labels Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XS 🚢 4.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants