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

chore: fix build of alertmanager without docker #2848

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jmichalek132
Copy link

@jmichalek132 jmichalek132 commented Mar 15, 2022

Hi, so we build alertmanager from source because of some internal requirements. We do this inside docker as part of multi stage build. We also don't have docker in docker available.
We were using the NO_DOCKER env variable to make the build not use docker.
However this does work on line 58, where DOCKER_RUN_CURRENT_USER is used instead DOCKER_CMD this one is not set to empty when NO_DOCKER env variable is set to true.
This PR fixes that and makes the build work without docker available again.

Signed-off-by: Juraj Michalek <jmichalek@medallia.com>
@jmichalek132 jmichalek132 force-pushed the jm-chore-fix-build-of-alertmanager-without-docker branch from b73bad1 to ce2504c Compare March 15, 2022 16:12
Signed-off-by: Juraj Michalek <jmichalek@medallia.com>
@jmichalek132
Copy link
Author

jmichalek132 commented Mar 16, 2022

Hi, @simonpasquier could you please take a look at this small change once you have some time?

Thank you in advance.

Copy link
Member

@metalmatze metalmatze left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

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

lgtm

@simonpasquier
Copy link
Member

oh wait! Would this work as you expect? When I tried locally, I get rm: invalid option -- 'g' which makes sense looking at this part.

$(DOCKER_RUN_CURRENT_USER) --rm -v ${PWD}/../..:/local openapitools/openapi-generator-cli:v3.3.4 generate \
-i /local/api/v2/openapi.yaml \
-g elm \
-o /local/ui/app/$(TEMPOPENAPI)

@jmichalek132
Copy link
Author

jmichalek132 commented Mar 16, 2022

oh wait! Would this work as you expect? When I tried locally, I get rm: invalid option -- 'g' which makes sense looking at this part.

$(DOCKER_RUN_CURRENT_USER) --rm -v ${PWD}/../..:/local openapitools/openapi-generator-cli:v3.3.4 generate \
-i /local/api/v2/openapi.yaml \
-g elm \
-o /local/ui/app/$(TEMPOPENAPI)

That's interesting, yeah I what you are point out by for some reason it doesn't happen for us in our pipeline.
Basically when I use the docker image we use for building alertmanager, set NO_DOCKER to true and run make scriptjs it works. I will look into it more.

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.

3 participants