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

add: Also tag the latest build for alpine #230

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

oliv3r
Copy link

@oliv3r oliv3r commented Mar 21, 2021

It is quite common, and upstream postgres does so as well, to also tag a 'latest' for alpine. In this case, this would result in postgis:alpine being the latest stable alpine release. This makes it easier (albeit riskier) to track the latest stable postgis release. The risk should be acceptable however, as the same risk would be taken when tracking postgis:latest so it really is up to the user.

It is quite common, and upstream postgres does so as well, to also tag a 'latest' for alpine. In this case, this would result in `postgis:alpine` being the latest stable alpine release. This makes it easier (albeit riskier) to track the latest stable postgis release. The risk should be acceptable however, as the same risk would be taken when tracking `postgis:latest` so it really is up to the user.
@oliv3r oliv3r force-pushed the dev/build_latest_alpine branch from f7789eb to a64f38f Compare March 21, 2021 22:01
@phillipross
Copy link
Contributor

While I like this idea, I think more work will need to be done to the Makefile in order to make things work correctly.

The logic in the Makefile assumes the rules involved with tagging the latest image will never include building the alpine variant, only the default. Thus, the required steps necessary to have an alpine image built may not have been met when the Makefile rules that do the tagging are invoked. The reason the CI builds doesn't fail is because these push/tag events are explicitly not invoked for pull requests or any branch other than the master.

I'll have to take a closer look, but I think the most straightforward way of doing this would be to have a separate set of rules for tagging the alpine variant with the "alpine" flag, or rework the existing rules to properly depend on the VERSION and VARIANT combinations to derive the cases where the latest variant image is built and the tag for that variant is used.

@oliv3r
Copy link
Author

oliv3r commented Mar 24, 2021

While I like this idea, I think more work will need to be done to the Makefile in order to make things work correctly.

The logic in the Makefile assumes the rules involved with tagging the latest image will never include building the alpine variant, only the default. Thus, the required steps necessary to have an alpine image built may not have been met when the Makefile rules that do the tagging are invoked. The reason the CI builds doesn't fail is because these push/tag events are explicitly not invoked for pull requests or any branch other than the master.

I'll have to take a closer look, but I think the most straightforward way of doing this would be to have a separate set of rules for tagging the alpine variant with the "alpine" flag, or rework the existing rules to properly depend on the VERSION and VARIANT combinations to derive the cases where the latest variant image is built and the tag for that variant is used.

Thanks for your reply @phillipross, and sorry for not responding sooner. I was looking at the bottom of the screen and the pipeline results are below your reply :)

I'm no Makexpert so I'll see if I can figure out what you meant with rework the existing rules to properly depend, as I didn't recognize those straight away.

My thought is, that if you have a (latest) release, you want to ideally indeed push them simultaneously, as you want them as a matching pair. So I'll try to spot what you meant ...

@phillipross
Copy link
Contributor

Thanks for your reply @phillipross, and sorry for not responding sooner. I was looking at the bottom of the screen and the pipeline results are below your reply :)

No problem, and thanks for contributing!

I'm no Makexpert so I'll see if I can figure out what you meant with rework the existing rules to properly depend, as I didn't recognize those straight away.

My thought is, that if you have a (latest) release, you want to ideally indeed push them simultaneously, as you want them as a matching pair. So I'll try to spot what you meant ...

The way the build is currently setup, the makefile supports setting VERSION and VARIANT environment vars specifying what versions and variants to be built. If they are omitted, then all will be built. This logic is encapsulated in the Makefile which unfortunately does obscure things for folks who are not so familiar with make. We make attempts to mitigate this with documentation and comments, but obviously we can't eliminate the learning curve entirely.

In cases where a build has only specified the default variant (debian) and not alpine, there will be no alpine images built, so attempts to push or tag them will fail. Rather than just add the commands to push and tag an alpine image in the same rule that does them for the default, a little more work will be necessary to only push/tag the alpine images in the cases where they are being built.

The other issue I foresee is that in cases where there might be a VERSION that does not have an alpine variant, the rules will need to somehow account for that case as well. The current builds don't contain any cases of this, but the roadmap does have items for implementing multiplatform images and/or arm builds, and the debian variants do not support the arm platforms for all versions. Again, this isn't a case that needs to be accounted for immediately, but it would be nice if the shorter term changes to the Makefile keep this case in mind.

So, with that in mind, if you don't mind traversing the learning curve involved with make, I'd be happy to review and offer feedback on whatever you can contribute 😉

@oliv3r
Copy link
Author

oliv3r commented Mar 28, 2021

So if I understand you correctly, you do prefer having them separate targets (based on the env variables) to offer the most flexibility?

I also hear what you are saying for the future cases, where you may even need more flexibility, but I guess getting to a 'single make' would be even harder in that case?

Let me dabble a bit and see what I can come up with first to wrap my mind around this :) In what you really want and whats the easiest to get going first round.

How do you trigger these commands btw? Are they run locally by a human, or are the tag/event based on pipelines that eventually do this?

I'm not complete stranger to make, but maybe estranged a little :p

@phillipross
Copy link
Contributor

So if I understand you correctly, you do prefer having them separate targets (based on the env variables) to offer the most flexibility?

How do you trigger these commands btw? Are they run locally by a human, or are the tag/event based on pipelines that eventually do this?

Ideally the commands can be triggered by humans during development/testing as well as triggered by the CI for eventual building, validating, and pushing the images. Currently the VERSION and VARIANT inputs are able to specifically invoke builds of specific versions and variants which in turn allows the CI to configure a build matrix and test finer-grained steps in the pipeline. This also allows developers to focus on specific combinations for the cases and conditions they are currently developing/debugging.

My feeling is that separate targets would be better as the code would be easier to read for people who are less familiar with Makefiles. But I understand that actively working to implement one approach often leads to exposing details that actually cause one to shy away and explore other better approaches. For this reason, I tend to ask contributors to consider my input and feedback as suggestions rather than prescriptions 😉

@ImreSamu ImreSamu mentioned this pull request Feb 23, 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