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

Implement workspace internal dependency updater #262

Merged
merged 4 commits into from
Sep 12, 2023

Conversation

EduardGomezEscandell
Copy link
Contributor

@EduardGomezEscandell EduardGomezEscandell commented Sep 6, 2023

This workflow will keep internal dependencies updated.

UDENG-267

PS: Sorry for the force-pushing. I was testing commit signing 🤭

@EduardGomezEscandell EduardGomezEscandell self-assigned this Sep 6, 2023
@EduardGomezEscandell EduardGomezEscandell force-pushed the update-workspace-dependencies branch from 49b516e to 371e1e3 Compare September 7, 2023 07:42
@EduardGomezEscandell EduardGomezEscandell requested review from didrocks and removed request for didrocks September 7, 2023 07:46
@EduardGomezEscandell EduardGomezEscandell force-pushed the update-workspace-dependencies branch from 371e1e3 to 3c54edb Compare September 7, 2023 07:50
@EduardGomezEscandell EduardGomezEscandell marked this pull request as ready for review September 7, 2023 07:50
@EduardGomezEscandell EduardGomezEscandell force-pushed the update-workspace-dependencies branch from 3c54edb to d2a2895 Compare September 7, 2023 09:02
@EduardGomezEscandell
Copy link
Contributor Author

@didrocks Was thinking about this. All our PR commits belong to the main branch because we use merge commits, so is this really doing anything useful?

It makes sense if you squash, since old commits become unreachable. But this is not the case here.
Maybe we could run this periodically (once a week?) to keep the imports up-to-date, but I'm not sure that would do anything either.

@EduardGomezEscandell EduardGomezEscandell force-pushed the update-workspace-dependencies branch from d2a2895 to 800c11a Compare September 7, 2023 11:04
@EduardGomezEscandell EduardGomezEscandell force-pushed the update-workspace-dependencies branch from 800c11a to f411abe Compare September 7, 2023 11:34
@didrocks
Copy link
Collaborator

didrocks commented Sep 8, 2023

I think the action should be more fine-grained, as if look at any of those common deps for last commit if there is any update in the go files. Then, only trigger the update hook.

However, thinking about it, the issue is about building the deb package, the rest is fine and good only be merged in the main branch. So, I’m wondering, could we trigger that on PR rather, check if there is any change in those common deps subdirectories, and in that case, run the action, and create a PR that is not automerged against your PR branch? WDYT?

Only the WSL-Pro-Service needs to stay up-to-date.
And we only need to update modules when these have changed
@EduardGomezEscandell EduardGomezEscandell force-pushed the update-workspace-dependencies branch from ebe0f2f to f9a8de4 Compare September 12, 2023 08:54
@EduardGomezEscandell
Copy link
Contributor Author

The way it works now:

  • It only checks WSL-Pro-Service
  • It looks at the dependency ref:
github.com/canonical/ubuntu-pro-for-windows/agentapi v0.0.0-20230906090052-60fb5d60ada4
                                            ^^^^^^^^                       ^^^^^^^^^^^^
                                            PATH                           COMMIT
  • It checks if there is any diff in that submodule since that commit:
git diff ${COMMIT} -- ${PATH}
  • If no diff, continue
  • Otherwise: update the dependency

Then, only push the branch if anything changed.

This method should considerably reduce spam, although it'll still have some false positives, such as when editing non-code files in the dependencies. If this ever happens to be an issue, we can filter diffs by file extension; however I doubt this'll ever become an issue.

Copy link
Collaborator

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

Apart from the internal vs external scripts and env variables passing silently as argument, I agree with the current approach.

Copy link
Collaborator

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

… and using that opportunity to clean up some trailing spaces, I see you! :)

@EduardGomezEscandell EduardGomezEscandell merged commit 3067c8b into main Sep 12, 2023
30 checks passed
@EduardGomezEscandell EduardGomezEscandell deleted the update-workspace-dependencies branch September 12, 2023 12:50
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.

2 participants