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

[MM-15717] build docker image #22

Merged
merged 4 commits into from
Jun 19, 2019
Merged

[MM-15717] build docker image #22

merged 4 commits into from
Jun 19, 2019

Conversation

cpanato
Copy link
Contributor

@cpanato cpanato commented Jun 5, 2019

Instead of build the tar.gz package we will only build the image.

  • every time merge to master the latest tag will be updated
  • every time we tag a release it will build the tag

@cpanato cpanato marked this pull request as ready for review June 5, 2019 14:35
@cpanato cpanato added the 2: Dev Review Requires review by a developer label Jun 5, 2019
@cpanato cpanato changed the title build docker image [MM-15717] build docker image Jun 5, 2019
@cpanato
Copy link
Contributor Author

cpanato commented Jun 7, 2019

Do we need other dependencies?

@gabrieljackson
Copy link
Contributor

Yeah, right now there are at least two other dependencies for running the provisioning server: kops and terraform. The code currently expects them to be in /usr/local/bin.

@cpanato
Copy link
Contributor Author

cpanato commented Jun 7, 2019

Will update
@gabrieljackson what are the versions required?

@gabrieljackson
Copy link
Contributor

Right now, I am running the latest versions that were out at the time I installed them:

gmbp ~/go/src/github.com/mattermost/mattermost-cloud (master) on$ kops version
Version 1.11.1
gmbp ~/go/src/github.com/mattermost/mattermost-cloud (master) on$ terraform version
Terraform v0.11.13

That said, I think we can probably use the newest versions that are out right now.

Copy link
Contributor

@gabrieljackson gabrieljackson 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. Will approve once terraform and kops are added.

Makefile Outdated Show resolved Hide resolved
build/bin/entrypoint Outdated Show resolved Hide resolved
Copy link
Member

@lieut-data lieut-data left a comment

Choose a reason for hiding this comment

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

Agree with @gabrieljackson's feedback, and a few more thoughts.

.circleci/config.yml Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
build/Dockerfile Outdated Show resolved Hide resolved
build/bin/user_setup Outdated Show resolved Hide resolved
# runtime user will need to be able to self-insert in /etc/passwd
chmod g+rw /etc/passwd

# no need for this script to remain in the image after running
Copy link
Member

Choose a reason for hiding this comment

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

This seems counter to how a script should be run: propose removing it in the caller instead of self-removing.

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 operator, just re-using the same thing here

Copy link
Member

Choose a reason for hiding this comment

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

I confess I haven't stayed up to date with the operator -- can you elaborate on what it's doing that makes that required here?

@cpanato
Copy link
Contributor Author

cpanato commented Jun 9, 2019

@gabrieljackson @lieut-data PTAL

do we need helm as well?

@stylianosrigas
Copy link
Contributor

@cpanato we do. The provisioner will run helm install for Prometheus and Fluentd.

@cpanato
Copy link
Contributor Author

cpanato commented Jun 10, 2019

@stylianosrigas helm added

@cpanato
Copy link
Contributor Author

cpanato commented Jun 10, 2019

will add kubectl

@gabrieljackson
Copy link
Contributor

We should get this merged into master now that we know it is working. Would it be possible to rebase and force push up one fresh commit to clean things up a bit?

@cpanato
Copy link
Contributor Author

cpanato commented Jun 14, 2019

we need to merge this first #31

@gabrieljackson
Copy link
Contributor

I wanted to note that #29 adds some new flags that are required to start the server.

@cpanato
Copy link
Contributor Author

cpanato commented Jun 17, 2019

this image is running on test account

@gabrieljackson @lieut-data @stylianosrigas PTAL

Copy link
Contributor

@stylianosrigas stylianosrigas 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
Contributor

@stylianosrigas stylianosrigas 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

@lieut-data lieut-data left a comment

Choose a reason for hiding this comment

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

Providing feedback, but lifting review to let others decide on the priority here.

Makefile Show resolved Hide resolved
# runtime user will need to be able to self-insert in /etc/passwd
chmod g+rw /etc/passwd

# no need for this script to remain in the image after running
Copy link
Member

Choose a reason for hiding this comment

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

I confess I haven't stayed up to date with the operator -- can you elaborate on what it's doing that makes that required here?

Makefile Outdated Show resolved Hide resolved
@cpanato
Copy link
Contributor Author

cpanato commented Jun 19, 2019

@lieut-data it is not required to have the build/bin/user_setup i thought this is ok to have, but i will confess that maybe is not need at all.

regarding the gopath I've removed that

@cpanato cpanato removed the 2: Dev Review Requires review by a developer label Jun 19, 2019
@cpanato cpanato added the 4: Reviews Complete All reviewers have approved the pull request label Jun 19, 2019
@cpanato cpanato merged commit 5b09172 into mattermost:master Jun 19, 2019
@cpanato cpanato deleted the MM-15717-new branch June 19, 2019 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants