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

feat(ci): add support for user-defined jobs #417

Merged
merged 1 commit into from
Sep 27, 2023
Merged

Conversation

mistydemeo
Copy link
Contributor

This adds support for user-defined jobs in CI. Currently, we support publish jobs specifically, and these run parallel with other publish jobs. These are specified in TOML via the user_publish_jobs hash:

user_publish_jobs = { github = ["./.github/workflows/something.yml"] }

Fixes #379.

@mistydemeo mistydemeo requested a review from Gankra September 8, 2023 01:50
@Gankra
Copy link
Contributor

Gankra commented Sep 11, 2023

For discussion, writing out the slug-driven approach I had pictured:

publish-jobs = ["itch"]

Would tell us that:

  • When building github CI, we expect to find a file called .github/workflows/publish-itch.yml
  • We generate a step called publish-itch in .github/workflows/release.yml containing that fragment

With a hypothetical buildkite workflow (i have no idea what buildkite looks like, let's pretend it's vaguely github-shaped):

  • When building buildkite CI, we expect to find a file called .buildkite/pipelines/publish-itch.yml
  • We generate a step called publish-itch in .buildkite/pipelines/release.yml containing the fragment

I also figured it would be more ergonomic to have builtins and user-defined jobs sharing the publish-jobs / upload-jobs array, but I respect wanting to make them separate arrays.

@mistydemeo
Copy link
Contributor Author

Hm, that slug approach seems reasonable. While it commits us to a specific directory structure in the target repo, that's not really a bad thing.

I do still want to keep it separated out by CI system. I can see a scenario where someone has more than one configured in the same repo and doesn't have 1:1 job parity between systems. Even if it requires more work from the user, I think it's better to specify extra jobs per CI system.

@Gankra
Copy link
Contributor

Gankra commented Sep 11, 2023

Can you envision an approach where the per-ci-system stuff can be sugarred away for people who don't need it?

@mistydemeo
Copy link
Contributor Author

Do you think that's necessary? Specifying a hash isn't enough extra work for the user that supporting a sugar syntax in addition to the other one feels like it's a bit more complex than we want to commit to.

@Gankra
Copy link
Contributor

Gankra commented Sep 11, 2023

tbh I'll be surprised if any user ever needs that level of generality, so I respect the desire to have it available but it seems like an ergonomics pain to force everyone to refer to the possibility but never use it.

@mistydemeo mistydemeo force-pushed the ci-plus-plus-plus branch 3 times, most recently from f1afaa2 to 1221dc2 Compare September 11, 2023 20:33
@mistydemeo mistydemeo marked this pull request as ready for review September 11, 2023 20:33
@mistydemeo
Copy link
Contributor Author

I got talked into the simpler list syntax in place of the more complex hash syntax; pushed a version with that. Think this is ready now, save docs.

@mistydemeo mistydemeo force-pushed the ci-plus-plus-plus branch 2 times, most recently from ac11e62 to d1655a9 Compare September 13, 2023 21:08
@mistydemeo
Copy link
Contributor Author

The most recent version places both types of publish jobs in a single array. We distinguish between types via prefixing user jobs with a @. For example:

publish-jobs = ["homebrew", "@greeter"]

homebrew is a cargo-dist builtin, and @greeter is mapped to .github/workflows/publish-greeter.yml.

@mistydemeo mistydemeo force-pushed the ci-plus-plus-plus branch 3 times, most recently from f25f7e5 to ddada20 Compare September 20, 2023 16:41
Copy link
Contributor

@Gankra Gankra left a comment

Choose a reason for hiding this comment

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

General impl seems right, just some gunk from how many iterations this went through to clean up

cargo-dist/src/errors.rs Outdated Show resolved Hide resolved
cargo-dist/src/tasks.rs Outdated Show resolved Hide resolved
cargo-dist/src/config.rs Outdated Show resolved Hide resolved
Comment on lines +723 to +728
let string = s.to_string();
if let Some(stripped) = string.strip_prefix("./") {
stripped.to_owned()
} else {
string
}
Copy link
Contributor

Choose a reason for hiding this comment

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

the ./ is stripped by this point in the code, so no need to map

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not true, the publish_jobs vec we're fetching from has the original prefixed names.

Copy link
Contributor

Choose a reason for hiding this comment

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

image

both versions of this code were stripping it, how are we ending up with multiple copies of the strip being needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question! I'm... not really sure? I checked and it's absolutely true that the FromStr is constructing the PublishStyle with no ./ prefix, while the type in the task does have it until I strip it here.

cargo-dist/templates/ci/github_ci.yml.j2 Outdated Show resolved Hide resolved
cargo-dist/templates/ci/github_ci.yml.j2 Show resolved Hide resolved
cargo-dist/tests/integration-tests.rs Outdated Show resolved Hide resolved
@Gankra Gankra added this to the 0.3.0 milestone Sep 20, 2023
@Gankra Gankra merged commit a3bdd5c into main Sep 27, 2023
14 checks passed
@Gankra Gankra deleted the ci-plus-plus-plus branch September 27, 2023 12:14
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.

release lifecycle plugins
2 participants