Skip to content

chore: Dockerfile - Simplify formatting SQL_ID #162

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

Merged
merged 1 commit into from
May 23, 2025
Merged

Conversation

polarathene
Copy link
Contributor

@polarathene polarathene commented May 23, 2025

The awk approach looked...awkward 🥁 This is more DRY 😎

This pipes the value into xargs that splits the SQLITE_VERSION string by . as the delimiter and passes those as args to printf command which is formatting the major.minor.patch values similar to how the original Python script did (or your individual printf formats with each one parsed via awk).

@polarathene
Copy link
Contributor Author

CI was triggered for the PR since I'm a contributor now after you merged my first PR. Secrets aren't available for such though, hence CI failing.

Personally you shouldn't have the workflow try to login/publish PRs, only build them.

Copy link
Owner

@clux clux left a comment

Choose a reason for hiding this comment

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

much nicer yeah 😅

thank you!

@clux
Copy link
Owner

clux commented May 23, 2025

Personally you shouldn't have the workflow try to login/publish PRs, only build them.

yeah, i am meant to fix that :D

@clux clux merged commit 6376e59 into clux:main May 23, 2025
2 of 6 checks passed
@polarathene
Copy link
Contributor Author

yeah, i am meant to fix that :D

For a project I maintain, I have simpler workflows like these:

I find that's a bit easier to grok and manage from a top-level. While they each share common reusable workflows that are more verbose:

Most other projects I see though just litter if conditionals on if it's a PR triggering the event to prevent doing actions like publishing the image, so it's up to you 😅

In that project there's a common build workflow, so PRs will build and test only AMD64 (if tests pass, only then do we bother with the ARM64 build which is quite slow), but the publish workflow will take a build (from cache if possible) and publish the image for both platforms.

@clux
Copy link
Owner

clux commented May 23, 2025

I find that's a bit easier to grok and manage from a top-level. While they each share common reusable workflows that are more verbose

yeah, like the idea of that.

Most other projects I see though just litter if conditionals on if it's a PR triggering the event to prevent doing actions like publishing the image, so it's up to you 😅

yeah.. it's the logical first step. i start hating the if conditionals when you start to need to && them together though.
btw ..i did add some dumb if conditions to the login/push steps and the merge workflow after merging your ci refactoring pr in the hopes that ci is more useful for you. #164

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