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

Move docker files to superset sub dir #59

Merged
merged 1 commit into from
May 23, 2019
Merged

Move docker files to superset sub dir #59

merged 1 commit into from
May 23, 2019

Conversation

carlosms
Copy link
Contributor

Fix #54.

The build now uses the superset/contrib/docker dir for the docker files, this way we can keep up with changes in upstream (there were a few small changes already that were missing from our Dockerfile).

After moving the docker related files I've realized that the .env file was setting the docker compose project name, but this is not very reliable. If you download the binary from our releases page you will not have this file. So I added the --project-name option to the docker-compose command. I also tested this for the compose inside a container.

@carlosms carlosms requested review from smacker and dpordomingo May 20, 2019 15:37
@@ -1,17 +0,0 @@
#
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you need to delete it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't rely on the contents of .env for a normal installation of sandbox-ce, so I wanted to notify this somehow here. If you add variables here, they will not have an effect on our final distribution.

@@ -135,5 +135,6 @@ func Run(ctx context.Context, arg ...string) error {
return err
}

return comp.Run(ctx, arg...)
newArgs := append([]string{"--project-name", "srcd"}, arg...)
Copy link
Contributor

Choose a reason for hiding this comment

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

github doesn't show conflict but current master already set project name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in master it was a different method, so there was no conflict but the result would be the same arg added twice. I have removed it here, rebased on top of master and squashed.

@@ -1,58 +0,0 @@
# Licensed to the Apache Software Foundation (ASF) under one
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should move our config here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was going to say no because right now superset_config and superset_config_dev are in the same dir. But moving it here allows us to detect if there's anything added in upstream, so makes sense. Done in df208b5.

@carlosms carlosms merged commit ab2f7f1 into src-d:master May 23, 2019
@carlosms carlosms deleted the mv-dockerfile branch May 23, 2019 17:19
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.

Move docker dir into superset subtree
2 participants