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

Add a GH workflow to update crates versions weekly #40

Closed
wants to merge 3 commits into from

Conversation

dhovart
Copy link
Contributor

@dhovart dhovart commented Dec 7, 2021

As discussed in #37

The supported crates are listed in local-registry/Cargo.toml under [dependencies].

I have updated the Dockerfile accordingly to remove unnecessary steps.

@dhovart dhovart requested a review from a team as a code owner December 7, 2021 14:25
@@ -7,3 +7,234 @@ edition = "2021"
path = "dummy.rs"

[dependencies]
unicode-segmentation = "1.8.0"
Copy link
Member

Choose a reason for hiding this comment

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

It's great to have the dependencies listed here explicitly 👍

.github/workflows/update-crate-versions.yml Outdated Show resolved Hide resolved
.github/workflows/update-crate-versions.yml Outdated Show resolved Hide resolved
.github/workflows/update-crate-versions.yml Outdated Show resolved Hide resolved
.github/workflows/update-crate-versions.yml Outdated Show resolved Hide resolved
echo "${{env.cargo_toml}}" > local-registry/Cargo.toml
- name: Create Pull Request
id: cpr
uses: peter-evans/create-pull-request@v3
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above. @exercism/github-actions what do you think about this action?

Note that you can also use the GitHub CLI, which comes pre-installed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, all right, I didn't know that the cli tools came pre-installed, I can create a PR this way if it's preferable.

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 it might be slightly less susceptible to misuse (or errors)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wondering if that would actually work since I'm specifying container to be rust:latest...

Copy link
Member

Choose a reason for hiding this comment

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

It won't. That said, it might be worth changing this workflow to be like https://github.com/exercism/rust/blob/main/.github/workflows/tests.yml#L113, which is the tests workflow for the rust repo. In it, the base image is ubuntu, but the action-rs action is used to install rust.

But let's wait for @exercism/github-actions to see what they think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what about now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello, it's been a few months and I'd like to know why you stopped reviewing, it was nice contributing to exercism :)

Copy link
Member

Choose a reason for hiding this comment

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

I'm really not the right person to review this, but at a guess, the people who'd been working with you on this PR got distracted and then just forgot.

Copy link
Member

Choose a reason for hiding this comment

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

I'm so sorry @dhovart! I guess @SaschaMann and I just forgot to review this. Our apologies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, no worries at all :)

@dhovart dhovart force-pushed the update-versions-workflow branch from ed0fb8e to 9402e68 Compare December 9, 2021 12:56
@dhovart
Copy link
Contributor Author

dhovart commented Dec 9, 2021

We should maybe add as an ultimate step "cargo build" from within local-registry to make sure all crates can be compiled without issue.

While working on #42 I saw that the docker build was failing because of some crates not correctly compiling.

default: true
- name: Retrieve newest versions of dependencies
run: ./bin/update-crates-versions.sh
- name: Create pull request
Copy link
Member

Choose a reason for hiding this comment

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

I would maybe split this one step into several steps, like https://github.com/exercism/org-wide-files/blob/main/.github/workflows/sync-rest.yml#L303 That makes it easier to debug what went wrong when a build fails.

Comment on lines +30 to +31
git config --global user.email "[email protected]"
git config --global user.name "Exercism Bot"
Copy link
Member

Choose a reason for hiding this comment

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

@SaschaMann will this work with the GITHUB_TOKEN passed in not being the token of the Exercism bot? And does it even need to be the Exercism Bot user here?

Choose a reason for hiding this comment

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

It might show up as unverified but generally the git name and email can be whatever. For consistency, you might want to set these to match Exercism Bot elsewhere, or at least point it either at an obvious dummy email address or one that actually works, not one that looks like it may work but likely doesn't.

https://github.com/exercism/org-wide-files/blob/601d9a5924d470499451e441371f9b3bd11caef0/.github/workflows/sync-rest.yml#L23-L24

Copy link

@SaschaMann SaschaMann left a comment

Choose a reason for hiding this comment

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

I know nothing about the Rust ecosystem but cargo is supported by Dependabot. Is there any reason to not use that instead?

Comment on lines +30 to +31
git config --global user.email "[email protected]"
git config --global user.name "Exercism Bot"

Choose a reason for hiding this comment

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

It might show up as unverified but generally the git name and email can be whatever. For consistency, you might want to set these to match Exercism Bot elsewhere, or at least point it either at an obvious dummy email address or one that actually works, not one that looks like it may work but likely doesn't.

https://github.com/exercism/org-wide-files/blob/601d9a5924d470499451e441371f9b3bd11caef0/.github/workflows/sync-rest.yml#L23-L24

@dhovart
Copy link
Contributor Author

dhovart commented Mar 26, 2022

I know nothing about the Rust ecosystem but cargo is supported by Dependabot. Is there any reason to not use that instead?

Ha, you're absolutely right @SaschaMann, for some reason I had not considered Dependabot, I didn't know it supported Rust projects. I suppose it's just a matter of configuring it correctly.
So if you're okay with that I will just make sure that we get an updated list of crates weekly using dependabot.

What would be for you the right way to proceed ? Should I force-push the same branch with only these changes ? Open a new PR ?

@SaschaMann
Copy link

I'd say it's best to open a new PR with the dependabot config

@ErikSchierboom
Copy link
Member

That sounds like a great option!

@dhovart
Copy link
Contributor Author

dhovart commented Apr 8, 2022

Sorry, I might need a week or two before being able to work on this.
In the meantime if anyone feels like working on this feel free to do so.

@senekor
Copy link
Contributor

senekor commented Jul 23, 2023

This was superceded by #77, which added a dependabot config to the existing work to update the crates as suggested in this discussion.

@senekor senekor closed this Jul 23, 2023
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.

5 participants