Av/feat/various

[AlexisVLRT]

Issue

resolves #51

Changes

• If no app engine service is deployed, a dummy default app will be deployed to occupy the default name and domain. This ensures that the URL of the MLFlow web server will always be https://mlflow-dot-{PROJECT_ID}.ew.r.appspot.com
• If a brand exists then it will be imported and used by one-click-mlflow. (Unsure if this is the behavior we want. Maybe we prefer to not touch the existing brand? Or give the user the possibility to choose?)
• If an oauth client already exists, use it instead of creating a new one.
• Replaced IAM bindings by members. This prevents loss of access for users on other services using IAP.
• Set force_destroy to true on buckets. This allows terraform to destroy the bucket + the data inside. Required if we want to properly test pushing artifacts then destroy the infra
• Added a makefile target to cleanup your local files when switching between dev projects.

Testing

I validated this version by:

• deploying on a new project (make one-click-mlflow) + testing pushing experiments and seeing them in the UI
• deploying again to validate idempotency (make one-click-mlflow) + testing pushing experiments and seeing them in the UI
• destroyed (make destroy)
• deployed again to validate imports of brand/appengine app (make one-click-mlflow) + testing pushing experiments and seeing them in the UI

Sorry for the bulky PR with too many changes.

[griseau]

Hey @AlexisVLRT Didn't have the time to go into the code in details I just had a quick look, nice work below are my few remarks :

• The url won't be https://mlflow-dot-{PROJECT_ID}.ew.r.appspot.com every time, it depends on the region of cloud run deployment (I don't remember how the region is implemented)
• As you said, I won't recommend to import existing brands since it already exists and might be managed by another service/person.
• The created default service is a flexible service. For billing purpose I would recommend creating a standard service instead

[AlexisVLRT]

@griseau

• Good point regarding the region, I hadn't considered that. Will update.

• I'll fix the brand thing, as well as adding a flow diagram as documentation since it's getting a little obscure.

• I chose to deploy as a flex GAE for now just because it was a little easier to just specify some dummy docker image than create an app engine yaml and all. I agree it would be better to have a regular one, scaled to 0. It will make a good first issue for someone to tackle. I'll let it as is for now.

Also for some reason I can't add you as a reviewer anymore. Maybe because you're not in the org anymore?

[griseau]

@AlexisVLRT Nice ! Looks like this package is getting stronger and stronger ;)

Hum, I guess it's the reason yes

[AlexisVLRT]

• I've created #58 for the implementation of regional deployments to stop bloating this PR further.
• I've created #56 for the app engine flex -> standard change.
• I made it so an existing brand will not be overwritten

Ready for review :)

[benoitgoujon]

If an oauth client already exists, use it instead of creating a new one.

Does it mean that we use the same OAuth Client Id and Client secret for multiple deployments (even if they are in the same project)?

[AlexisVLRT]

Does it mean that we use the same OAuth Client Id and Client secret for multiple deployments

With the current implementation, yes. As far as I can tell, you can only have one oauth client/secret for all the app engine services on a project since it's defined at the google_app_engine_application level in Terraform.

[benoitgoujon]

Alright.

And you already answered the question that was following up. So good for me 👍