Skip to content

Conversation

itskingori
Copy link
Member

Package v2.1 with Alpine 3.4 base image and configure tests and automatic builds.

Copy link
Member

@zacblazic zacblazic left a comment

Choose a reason for hiding this comment

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

Few things need to be addressed.

README.md Outdated
# Docker OAuth2 Proxy

This is a small docker image for `oauth2_proxy` which is a reverse proxy
that provides authentication with Google, Github or other providers.
Copy link
Member

Choose a reason for hiding this comment

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

Casing here of Github should be GitHub.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 30f9424.


Get a `bash` shell from the container

$ docker run zappi/oauth2_proxy --version
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't get us a bash shell.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 9d1db07.

tagged:
tag: /.*/
commands:
- docker login -e $DOCKER_EMAIL -u $DOCKER_USER -p $DOCKER_PASS
Copy link
Member

Choose a reason for hiding this comment

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

Will these environment variables be set up as secrets in Circle CI?

Copy link
Member Author

Choose a reason for hiding this comment

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

I dunno. 😰

I'm also wondering who's details should I use.

commands:
- docker login -e $DOCKER_EMAIL -u $DOCKER_USER -p $DOCKER_PASS
- docker tag $IMAGE_NAME:latest $IMAGE_NAME:$CIRCLE_TAG
- docker push $IMAGE_NAME:$CIRCLE_TAG
Copy link
Member

Choose a reason for hiding this comment

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

Does this also push the latest tag?

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see you've added it in another commit. Can they not be pushed together?

Copy link
Member Author

Choose a reason for hiding this comment

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

I could but I'm not sure it works i.e. does it overwrite? will it error out due to a conflict like ECR?

Copy link
Member Author

@itskingori itskingori Sep 16, 2016

Choose a reason for hiding this comment

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

In that case I'd like to revert just that specific change.

Copy link
Member

Choose a reason for hiding this comment

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

I think let's just have it push the Git tag and see what happens first?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think let's just have it push the Git tag and see what happens first?

Dropped latest push for now. We can do it later.

Copy link
Member

@zacblazic zacblazic left a comment

Choose a reason for hiding this comment

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

Minor change.

circle.yml Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Not necessary to put :latest here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you sure?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is how we tag in deploy.sh:

docker tag $IMAGE_NAME $AWS_ACCOUNT_ID.dkr.ecr.$AWS_REGION.amazonaws.com/$IMAGE_NAME:$VERSION

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 7619ae6. However, it would have still worked. Docker defaults to latest when no tag set.

Copy link
Member

@zacblazic zacblazic left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@itskingori itskingori merged commit 1c289df into master Sep 16, 2016
@itskingori itskingori deleted the package branch September 16, 2016 12:37
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