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

Refactor (ci): Update .github/workflows/build.yml code #5

Conversation

joeltimothyoh
Copy link
Member

No description provided.

@joeltimothyoh joeltimothyoh self-assigned this Oct 4, 2021
@joeltimothyoh joeltimothyoh added this to the next-release milestone Oct 4, 2021
@joeltimothyoh
Copy link
Member Author

lgtm

@joeltimothyoh joeltimothyoh merged commit a39ab6d into startersclan:master Oct 4, 2021
@leojonathanoh
Copy link
Member

@joeltimothyoh looking at this now, the templating using matrix and env vars makes the build job look much more complex than necessary.

@joeltimothyoh
Copy link
Member Author

At least it would be maintainable but potential forkers of the repository, unlike having a dependency on code in and experience with PowerShell. @leojonathanoh

@leojonathanoh
Copy link
Member

@joeltimothyoh i don't mean to use an external templator. if templating is not necessary at all, as in this repo, then simply hardcoding the values will be sufficient in the build job.

@joeltimothyoh
Copy link
Member Author

Matrices again are for the populating build environments with the necessary variable values for building however number of variants at scale. @leojonathanoh

@leojonathanoh
Copy link
Member

leojonathanoh commented Oct 5, 2021

@joeltimothyoh matrices is simply a templator.

to me, the Dockerfile should be completely self-contained (no ARGs, so that any consumer simply performs docker build -f Dockerfile to get the desired image) The other ci job options not pertaining to docker build command, are ci variables, and for ci logic it's beter to use a script (e.g. ./build/build.sh that can also be used outside the ci runner.

The issue with using matrices, is that one puts the Dockerfile values inside a templator (ci config file), where the Dockerfile is now no longer self-contained, and the ci config file cannot be used in a development environment. So this pattern is not good for consumers.

@joeltimothyoh
Copy link
Member Author

I say it depends on use case. Pertaining to this project and it being OSS on GitHub, it suffices to use matrices with GHA.

This isn’t to say no improvements need to be made. I agree variables should be explicitly declared and abstractions done away with where possible. @leojonathanoh

@leojonathanoh
Copy link
Member

@joeltimothyoh for a simple repos with so few variants, it's better to make self-contained Dockerfile, and there is no need for matrices in github workflows to keep things simple.

@leojonathanoh
Copy link
Member

leojonathanoh commented Oct 6, 2021

this repo also doesn't have release job and changelog. those are very important too. if a release is a calver-based release, then a date-based tag should be dynamically determined, and used for the build jobs to determine the docker image tag.

Hence a "simple" repo is usually one only has code and a manual ci release process, but with many other essential ci elements being missing e.g. automated releases, changelog, etc. If it's open source, the repo shouldn't actually be "simple".

@joeltimothyoh
Copy link
Member Author

Opened #7

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

Successfully merging this pull request may close these issues.

None yet

2 participants