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

Deploy some more hub automatically #569

Closed
wants to merge 3 commits into from
Closed

Conversation

yuvipanda
Copy link
Member

@yuvipanda yuvipanda commented Jul 30, 2021

Ref #507

I'm worried that incomplete tests might leave notebook
nodes running, but the culler should take care of those? We
must verify this.

Added automatic deployment for everything except farallon
and openscapes hubs - those are using kops and I think
their kubernetes credentials will have expired. #381
tracks a fix.

Once this is merged, PRs like #580
can be deployed without needing a local setup

@yuvipanda
Copy link
Member Author

I thought I had named this branch cp-cd but apparently I was too sleepy to get it right

@choldgraf
Copy link
Member

I'm a bit confused - I though all hubs were auto deployed but maybe that is just for the pilot hubs?

It sounds like I need to do another deep dive in the pilot docs since it has been a while and they've changed a lot

@yuvipanda
Copy link
Member Author

@choldgraf Yeah, this repo basically deploys all our hubs now. I opened #570 to rename this repo to clarify.

@sgibson91 sgibson91 mentioned this pull request Aug 4, 2021
2 tasks
Ref 2i2c-org#507

I'm worried that incomplete tests might leave notebook
nodes running, but the culler should take care of those? We
must verify this.

The creds for the carbonplan cluster are from EKS,
and I'm not sure how long they are valid for either.
@yuvipanda yuvipanda changed the title Deploy carbonplan hub automatically Deploy some more hub automatically Aug 5, 2021
Copy link
Member

@sgibson91 sgibson91 left a comment

Choose a reason for hiding this comment

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

LGTM!

- requirements.txt
- dev-requirements.txt
- config/secrets.yaml
- config/hubs/carbonplan.cluster.yaml
Copy link
Member

@sgibson91 sgibson91 Aug 5, 2021

Choose a reason for hiding this comment

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

Is handling this "run on changes to a specific cluster.yaml file" the crux of the reason we are using workflow files per cluster rather than one file with a matrix job?

Copy link
Member Author

Choose a reason for hiding this comment

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

@sgibson91 It wasn't intentional - i was just following the pattern @GeorgianaElena set up. It is nice to not run them when there's no changes though.

Copy link
Member

@sgibson91 sgibson91 Aug 5, 2021

Choose a reason for hiding this comment

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

For sure! I was going to suggest using something like this action to set a conditional on the paths changed https://github.com/dorny/paths-filter
So we maintain the same behaviour of not doing a deploy when nothing's changed, but we can consolidate into a single workflow file and new clusters can just be added to the matrix element list.

Copy link
Member

Choose a reason for hiding this comment

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

(This shouldn't block the PR btw - merge away and we can open an issue to track this discussion if you like)

.github/workflows/deploy-pangeo-181919.yaml Outdated Show resolved Hide resolved
Copy link
Member

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

This looks good to me! I had the same thought that @sgibson91 did and I see she opened up #581 so I'm +1 on merging this and tracking that improvement there

@sgibson91
Copy link
Member

sgibson91 commented Aug 5, 2021

I've just opened #582 which would replace this PR if merged because it already contains the clusters @yuvipanda was adding in here

@choldgraf
Copy link
Member

Now that #582 has been merged, I think this one can be closed, so closing this but please reopen if I'm wrong!

@choldgraf choldgraf closed this Aug 5, 2021
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.

3 participants