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

Minor CI improvements #4266

Merged
merged 12 commits into from
Dec 14, 2023
Merged

Minor CI improvements #4266

merged 12 commits into from
Dec 14, 2023

Conversation

Nutomic
Copy link
Member

@Nutomic Nutomic commented Dec 14, 2023

Updated:

Locally Im getting 42s for running sql_format_check.sh, but only 16s for find -exec. Problem is that the repo files are owned by root, and I dont see any way to run steps as another user in Woodpecker.

Edit: Switched from pgformatter image to perl so that it can run as root. However the apt install seems too slow so it still takes 60s+ for the step. Previously it was 1m40s so a bit faster.

.woodpecker.yml Outdated
- apt install git
- find migrations -type f -name '*.sql' -exec pg_format -i {} +
# ensure that formatting doesn't result in any git changes
- git diff --exit-code
Copy link
Member

Choose a reason for hiding this comment

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

git isn't the right way to handle this, because it would fail on a new migration. I'd recommend just editing the format_check.sh function to use find exec.

Copy link
Member Author

Choose a reason for hiding this comment

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

No thats fine, git diff will only give exit code 1 if there are uncommitted changes. So if you add migrations in your PR they are part of a commit, and exit code is 0.

Find exec is complicated because then you need a different temp file for each input. Its annoying that pg_format doesnt have a --check option.

Copy link
Member

Choose a reason for hiding this comment

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

But how is it going to make sure the new migration that got added is correct sql?

Copy link
Member

Choose a reason for hiding this comment

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

Another easy way I can think of to speed it up, is in the sql_format_check script, use gnu parallel and make sure the tmp file is a new random one each time.

@Nutomic
Copy link
Member Author

Nutomic commented Dec 14, 2023

I noticed that drone-cache was silently broken, restarting minio server fixed it. To prevent this in the future, Im adding exit_code flag so that we notice future failures.

@dessalines
Copy link
Member

I added a format check that can use parallelism now: as long as it fails, you can merge this one into yours.

#4268

@Nutomic Nutomic changed the title Speed up SQL formatting in CI with parallel processing Minor CI improvements Dec 14, 2023
@dessalines dessalines enabled auto-merge (squash) December 14, 2023 16:06
@dessalines dessalines merged commit af4d008 into main Dec 14, 2023
@dessalines
Copy link
Member

Not sure how this passed the CI, but its breaking some other PRs. I'll revert for now.

dessalines added a commit that referenced this pull request Dec 14, 2023
dessalines added a commit that referenced this pull request Dec 14, 2023
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