-
Notifications
You must be signed in to change notification settings - Fork 2
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
pipeline health POC #59
base: main
Are you sure you want to change the base?
Conversation
1. Importing them all by hand, some code duplication and effort, but probably the least likely to blow up 2. Looping through them all We can also start with 1, and then move to 2 once everything is captured in the Pulumi state with 1(which seems like the sane option)
28320ba
to
bd199e9
Compare
Looking very promising! The old website PHP code for this is here: https://github.com/nf-core/website/blob/old-site/public_html/pipeline_health.php That contains all of the logic for stuff like the repo website, the set of minimum keywords and the types of things to do (eg. stripping old bad GHA job names). See the functions called |
Also this is the new pipeline health page in Astro, if it helps: https://github.com/nf-core/website/blob/main/sites/pipelines/src/pages/pipeline_health.astro That reports on a lot of this stuff but doesn't set anything. |
Ah, sweet! |
pulumi env run nf-core/github-prod -i pulumi import github:index/repository:Repository nf-core testpipeline
This is the best I'm gonna do. We can iterate in a readable way here.
pulumi import github:index/branchDefault:BranchDefault branch_default_testpipeline testpipeline
pulumi env run nf-core/github-prod -i pulumi import github:index/branch:Branch branch_dev_testpipeline testpipeline:dev
pulumi import github:index/repositoryRuleset:RepositoryRuleset ruleset_branch_default_testpipeline testpipeline:1220601
pulumi import github:index/repositoryRuleset:RepositoryRuleset ruleset_branch_dev_testpipeline testpipeline:1220600
pulumi import github:index/repositoryRuleset:RepositoryRuleset ruleset_branch_TEMPLATE_testpipeline testpipeline:1220597
@ewels Ignore the mypy errors will fix that in post. I think this is ready to review and 🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some thoughts
@@ -0,0 +1,5 @@ | |||
config: | |||
github:owner: nf-core-tf | |||
# https://start.1password.com/open/i?a=O5GICFDKPNABLLVGMKBL5JWDWA&v=rdfcz6oy6qxxrc4clu467a7dmm&i=4ajrv44kc5lcbboa37fr5oydla&h=nf-core.1password.eu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this throws an error for me. is that maybe for your personal account?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the link, you also we're in the Dev
vault. Had to make a vault that was specifically accessible to the service accounts, and I didn't want to give them access to everything.
pulumi/github/repos/Pulumi.dev.yaml
Outdated
github:owner: nf-core-tf | ||
# https://start.1password.com/open/i?a=O5GICFDKPNABLLVGMKBL5JWDWA&v=rdfcz6oy6qxxrc4clu467a7dmm&i=4ajrv44kc5lcbboa37fr5oydla&h=nf-core.1password.eu | ||
github:token: | ||
secure: AAABAFMgBNyCNuYsps6YVPV2L7Ji5qBJj0omEQQa9HrdhT2iHo3ex0e9NsDER3Q04itGiY698X/ZQCnTM2zu9op3tcjmzfITdHxGy0FGATuUFamYsSiztHrNAKiIEJ9E0M4Al8/yJeB6X4BXvkLEgik/I+GPvZIXK3tE65Q= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
secure: AAABAFMgBNyCNuYsps6YVPV2L7Ji5qBJj0omEQQa9HrdhT2iHo3ex0e9NsDER3Q04itGiY698X/ZQCnTM2zu9op3tcjmzfITdHxGy0FGATuUFamYsSiztHrNAKiIEJ9E0M4Al8/yJeB6X4BXvkLEgik/I+GPvZIXK3tE65Q= | |
secure: AAABAFMgBNyCNuYsps6YVPV2L7Ji5qBJj0omEQQa9HrdhT2iHo3ex0e9NsDER3Q04itGiY698X/ZQCnTM2zu9op3tcjmzfITdHxGy0FGATuUFamYsSiztHrNAKiIEJ9E0M4Al8/yJeB6X4BXvkLEgik/I+GPvZIXK3tE65Q= |
should we set this as a github secret?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can, the reason this one isn't is because I was struggling with the 1password Pulumi ESC integration, and I didn't realize you have to copy the plain service key into the environment file, and then it encrypts it in place for that specific environment file.
Anyways there's a few options:
- GitHub Secret
- Pulumi ESC
- Encrypting them in place like so(idk if you could run this for example or not)
This one doesn't really matter, because it's just to the nf-core-tf
account. I can update it to use Pulumi ESC.
Leaning Pulumi ESC for now as:
- That gives us a better access management for the secrets.
- It also allows you to develop locally easily, instead of pushing to GitHub anytime you want to preview the changes.
- Already have 1Password integration setup with it (So you just pull the secrets in from there instead of copying them, which allows you to roll and update them all in one place)
We could do all of that with GitHub actions, and pass all of these things, but the secret management is already a complicated web, but it's working currently.
TL;DR something to explore, I'll update this one and move it to Pulumi ESC though.
Co-authored-by: Matthias Hörtenhuber <[email protected]>
Co-authored-by: mashehu <[email protected]>
Need to check that we don't clobber certain fields, such as repository descriptions if already written. Similar for repo keywords: should have a minimal set but not clobber custom ones. This will vary across fields. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs some cleanup
|
||
- name: PR previews | ||
if: ${{ github.event_name == 'pull_request' }} | ||
uses: pulumi/actions@v3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uses: pulumi/actions@v3 | |
uses: pulumi/actions@v6 |
|
||
- name: Apply infrastructure update | ||
if: ${{ github.event_name == 'push' }} | ||
uses: pulumi/actions@v3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uses: pulumi/actions@v3 | |
uses: pulumi/actions@v6 |
- main | ||
paths: | ||
- "pulumi/github/repos/**/*" | ||
pull_request: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pull_request: | |
workflow_dispatch: | |
pull_request: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to handle core repos with pulumi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think not with the "loop" of all of the pipelines. Just as their own individual settings.
I think it can't hurt to import the settings.
"pairgenomealign", | ||
"phaseimpute", | ||
"reportho", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"pairgenomealign", | |
"phaseimpute", | |
"reportho", |
all these are released by now, so better not touch them 🙂
nfcore_testpipeline = github.Repository( | ||
NAME, | ||
description="A small example pipeline used to test new nf-core infrastructure and common code.", # 'repo_description' => 'Description must be set', | ||
has_downloads=True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
has_downloads=True, | |
has_downloads=False, |
|
||
CORE_TEAM_ID = 2649377 | ||
MAINTAINERS_TEAM_ID = 4462882 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
repo = github.Repository.get(pipeline, f"nf-core/{pipeline}") |
let's first query the old repo, to not overwrite certain fields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think pulumi has protections for that.
), | ||
), | ||
visibility="public", | ||
topics=TOPICS, # 'repo_keywords' => 'Minimum keywords set', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
topics=TOPICS, # 'repo_keywords' => 'Minimum keywords set', | |
topics= repo.topics ?? TOPICS, # 'repo_keywords' => 'Minimum keywords set', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we add missing topics without deleting the existing ones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, that's probably the better approach. the problem is that repo.topics
is not just an array, but a pulumi Output object...
@@ -0,0 +1,3 @@ | |||
pulumi>=3 | |||
pulumi_github>=5.20.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pulumi_github>=5.20.0 | |
pulumi_github>=6.6.0 |
push: | ||
branches: | ||
- main | ||
paths: | ||
- "pulumi/github/repos/**/*" | ||
pull_request: | ||
branches: | ||
- main | ||
paths: | ||
- "pulumi/github/repos/**/*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to understand. This will run on PRs to ops
, right? Is this action updating the pipeline health? And are these PRs on ops
related to pipeline health?
description="A small example pipeline used to test new nf-core infrastructure and common code.", # 'repo_description' => 'Description must be set', | ||
has_downloads=True, | ||
has_issues=True, # 'repo_issues' => 'Enable issues', | ||
has_projects=True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't have this for pipelines?
has_projects=True, | |
has_projects=False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a lot of them do actually.
), | ||
), | ||
visibility="public", | ||
topics=TOPICS, # 'repo_keywords' => 'Minimum keywords set', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we add missing topics without deleting the existing ones?
# 'branch_master_exists' => 'master branch: branch must exist', | ||
branch_default_testpipeline = github.BranchDefault( | ||
f"branch_default_{NAME}", | ||
branch="master", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we use main
already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I think we'll just loop through the pipelines they shouldn't have any special config.
), | ||
), | ||
enforcement="active", | ||
name="master", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
main
?
actor_type="Team", | ||
bypass_mode="always", | ||
) | ||
# TODO 'branch_template_restrict_push' => 'Restrict push to TEMPLATE to @nf-core-bot', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need this, everyone can run nf-core sync
manually with their github user
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just going for 1 to 1 with pipeline_health.
If I got that wrong, cool, but I vote we give our best attempt to get it the same, then make a PR to update those changes we can get rid of to "self-document" the change.
ClosesA start on #5I expect it to go through some iteration before actually using it to manage the pipelines, and even then a slow roll-out.
Just looking for anything that anyone absolutely hates, or settings that we can catch that are wrong.