Skip to content

Conversation

@Squareys
Copy link
Contributor

@Squareys Squareys commented Jun 3, 2020

As per emscripten-core/emscripten#11078 and #372.

@trzecieu @sbc100
Very very rough draft for building the docker image on Circle CI.

TODO

  • Build docker image
  • Properly specify the tag(s) for docker hub
  • Trigger build only on emscripten-releases-tags.txt changes. tags
    • Double check that this is really just triggering on tags.
  • What's the process of getting the docker hub deploy key? They should be specified through secret variables in Circle CI and probably could be done as a final step by one of the project maintainers.
  • Build various variations of the image? (i.e. I remember emscripten-slim and a ubuntu based version in trzeci's original images.)

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Very excited to see progress here.

- checkout
- run: docker build -t emscripten-core/emsdk:$CIRCLE_BRANCH docker/
- run: |
echo "$DOCKER_PASS" | docker login --username $DOCKER_USER --password-stdin
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the recommended way to authenticate the push ? I wonder if we can pass login credentials directly to the push command somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is from the CircleCI docs, yes. It is definitely possible to pass the password directly:

docker login -u "$DOCKER_USER" -p "$DOCKER_PASS" ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Pattern from the CircleCI docs, seems fine

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks odd, but also I've found that the recommended - more secure way for login.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Error: Cannot perform an interactive login from a non TTY device

Looks like --password-stdin is not accepted here anyways? 🤔

machine: true
steps:
- checkout
- run: docker build -t emscripten-core/emsdk:$CIRCLE_BRANCH docker/
Copy link
Collaborator

Choose a reason for hiding this comment

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

We do have docker.io organization called emscripten/emsdk.
I don't know who manages this organisation, looks like @haraldreingruber has access to another project emscripten/emscripten-ci - maybe he knows more :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I seem to remember deciding on emscripten/emsdk for this image, right? Or am I remembering wrong?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Most likely you're right. And looking at the name of this repo it makes sense.
In this case there should be a following action on https://hub.docker.com/orgs/emscripten

Copy link
Contributor

Choose a reason for hiding this comment

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

The org (https://hub.docker.com/orgs/emscripten) was created by @kripken, right?
It's odd, but I don't see any other members...

@Squareys
Copy link
Contributor Author

Squareys commented Jun 4, 2020

Trigger build only on emscripten-releases-tags.txt changes.

@sbc100 It looks like whenever this file changes, emsdk is tagged. Could we instead make this job trigger on tags instead? That would be very straight forward to achieve.

*edit: I implemented this suggestion now. Unsure whether this works, though, since CircleCI seems a bit confusing in this regard and the job has been triggered by my last push, despite not being a tag.

@Squareys Squareys force-pushed the circle-ci-build-docker-image branch 9 times, most recently from 1fbc527 to 648f665 Compare June 4, 2020 14:00
@Squareys Squareys marked this pull request as ready for review June 4, 2020 14:01
@Squareys Squareys force-pushed the circle-ci-build-docker-image branch from 648f665 to fa7c2d9 Compare June 4, 2020 14:03
@Squareys
Copy link
Contributor Author

Squareys commented Jun 4, 2020

From my side this is pretty much ready, what's left is:

  • Ensure this is running only on tags (I set up a filter which should make the job ignored for all branches, but included for all tags). The $CIRCLE_TAG variable is only set for tags, hence it fails for this PR.
  • Set DOCKER_PASS and DOCKER_USER secret variables in the Circle CI settings after merge.

So, this is ready to be merged from my side.

@Squareys Squareys requested a review from sbc100 June 4, 2020 14:08
@Squareys Squareys changed the title WIP: Build docker image on Circle CI Build docker image on Circle CI Jun 4, 2020
@Squareys Squareys force-pushed the circle-ci-build-docker-image branch from fa7c2d9 to 2774148 Compare June 4, 2020 19:56
@Squareys Squareys requested a review from sbc100 June 5, 2020 08:58
@Squareys
Copy link
Contributor Author

Squareys commented Jun 5, 2020

@sbc100 I fixed or commented on the issues you found. Tell me if there is anything else I can do.

@sbc100
Copy link
Collaborator

sbc100 commented Jun 5, 2020

lgtm.

What are the next steps? It looks like the CI is currently failing... and I guess we need to figure out how to inject the credentials?

@Squareys
Copy link
Contributor Author

Squareys commented Jun 5, 2020

Injecting the credentials should be done after merge by someone who has access to the Circle CI backend and the docker hub namespace (@kripken? See #513 (comment)).

The CI is failing because it is not running for a tag, hence CIRCLE_TAG variable is missing aswell. Once the PR is merged, it will only run on tags.

Also, I guess this needs to be run once for all the existing previous tags. Probably sufficient to do that manually.

@sbc100
Copy link
Collaborator

sbc100 commented Jun 5, 2020

The CI is failing because it is not running for a tag, hence CIRCLE_TAG variable is missing aswell. Once the PR is merged, it will only run on tags.

It seems a little odd to me that this would run on this PR at all.. if its only supposed to run on tags.. why is it running here?

@Squareys Squareys force-pushed the circle-ci-build-docker-image branch 2 times, most recently from 1705baa to bff9fe3 Compare June 5, 2020 22:15
@Squareys Squareys force-pushed the circle-ci-build-docker-image branch from bff9fe3 to 43b221d Compare June 5, 2020 22:17
@Squareys
Copy link
Contributor Author

Squareys commented Jun 5, 2020

It seems a little odd to me that this would run on this PR at all.. if its only supposed to run on tags.. why is it running here?

Uh, I figured it out, it needed 2 more spaces of indentation...

@sbc100 sbc100 merged commit 24d8848 into emscripten-core:master Jun 5, 2020
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.

4 participants