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

pipeline health POC #59

Open
wants to merge 40 commits into
base: main
Choose a base branch
from
Open

pipeline health POC #59

wants to merge 40 commits into from

Conversation

edmundmiller
Copy link
Contributor

@edmundmiller edmundmiller commented Jul 20, 2024

Closes A start on #5

I 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.

@edmundmiller edmundmiller self-assigned this Jul 20, 2024
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)
@ewels
Copy link
Member

ewels commented Jul 21, 2024

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 fix_* (mostly).

@ewels
Copy link
Member

ewels commented Jul 21, 2024

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.

@edmundmiller
Copy link
Contributor Author

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
@edmundmiller edmundmiller changed the title pipeline health pipeline health POC Jul 22, 2024
@edmundmiller edmundmiller marked this pull request as ready for review July 22, 2024 03:16
@edmundmiller edmundmiller requested a review from a team as a code owner July 22, 2024 03:16
@edmundmiller
Copy link
Contributor Author

@ewels Ignore the mypy errors will fix that in post. I think this is ready to review and 🚀

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,
Copy link
Member

Choose a reason for hiding this comment

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

I think projects should be disabled for pipeline repos, off the top of my head 🤔 Though there are maybe some exceptions? Not sure. May need discussion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which one? The has downloads? Yeah, that was just a default that it pulled in, I think.

I think we merge, call this a start, and then PRs can be opened based off those. In the next PR I'm going to abstract it out to a function for a pipeline, and then we can just call those from the "pipelines.json".

Aside:
Might need a list of descriptions though... 🤔

Copy link
Contributor

@mashehu mashehu left a comment

Choose a reason for hiding this comment

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

Some thoughts

.github/workflows/repos.yml Show resolved Hide resolved
.github/workflows/repos.yml Outdated Show resolved Hide resolved
docs/1password.md Outdated Show resolved Hide resolved
docs/1password.md Show resolved Hide resolved
docs/1password.md Outdated Show resolved Hide resolved
docs/1password.md Show resolved Hide resolved
@@ -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
Copy link
Contributor

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?

Copy link
Contributor Author

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.

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=
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
secure: AAABAFMgBNyCNuYsps6YVPV2L7Ji5qBJj0omEQQa9HrdhT2iHo3ex0e9NsDER3Q04itGiY698X/ZQCnTM2zu9op3tcjmzfITdHxGy0FGATuUFamYsSiztHrNAKiIEJ9E0M4Al8/yJeB6X4BXvkLEgik/I+GPvZIXK3tE65Q=
secure: AAABAFMgBNyCNuYsps6YVPV2L7Ji5qBJj0omEQQa9HrdhT2iHo3ex0e9NsDER3Q04itGiY698X/ZQCnTM2zu9op3tcjmzfITdHxGy0FGATuUFamYsSiztHrNAKiIEJ9E0M4Al8/yJeB6X4BXvkLEgik/I+GPvZIXK3tE65Q=

should we set this as a github secret?

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 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:

  1. GitHub Secret
  2. Pulumi ESC
  3. 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:

  1. That gives us a better access management for the secrets.
  2. It also allows you to develop locally easily, instead of pushing to GitHub anytime you want to preview the changes.
  3. 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.

pulumi/github/repos/Pulumi.yaml Outdated Show resolved Hide resolved
pulumi/github/repos/README.md Outdated Show resolved Hide resolved
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