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

chore: run examples in on-pull-request workflow #289

Merged
merged 3 commits into from
Mar 17, 2023

Conversation

poppoerika
Copy link
Contributor

  • Added a step to run the examples except the loadgen (I'm not sure if we want to run the script every time...) to make sure all the examples are up-to-date.
  • Updated a few examples to add cache cleanup.
  • Confirmed that dependabot is looking for new versions of the dependencies.

Closes #283

@pgautier404
Copy link
Contributor

pgautier404 commented Mar 16, 2023

I'm raising a flag here because we just went through this when a workflow to verify examples on pull requests was previously added (cc @eaddingtonwhite). It caused a ton of havoc and we ended up reverting the change. I really don't think we want to gate PRs on whether or not the examples build. We pin the SDK version in the examples exactly so we know that they work with the release they're pointed at, and we generally have a spike to bring examples up to spec each time we cut a new release. There shouldn't be an expectation in my opinion that the examples will always work with the HEAD of the repo.

@poppoerika
Copy link
Contributor Author

poppoerika commented Mar 16, 2023

@pgautier404
Thank you for the information!
I was not aware of the previous work on verifying the examples, and the change was reverted.
We created this ticket since I mentioned in one the PRs that we might be able to check examples to make sure they are up-to-date with charges in PRs we're filing (we had a lot of breaking changes recently prior to 1.0).
To me it seems like examples were not updated with those breaking changes recently hence the comment.
With that being said, I'm okay with not adding the step to check the examples every time if it caused many issues. (I want to make this change if it improves our process not worsen it...!!)

@cprice404
Copy link
Contributor

  • we should be verifying that examples build on every PR, BUT
  • they should be building against a pinned version, AND
  • we should have dependabot configured to detect when a new version is released, and file a PR to update the examples to the new version. when this happens the dependabot PR SHOULD be red if the examples no longer build against the new release, so it will draw our attention to it and we can fix it.

i haven't had a chance to look at the code changes in this PR but that ^^ is the state of the art for how we are doing examples for now.

@poppoerika
Copy link
Contributor Author

@cprice404 Thank you for the explanation!
I don't think we ever had a build step in our workflows although we already have a make build command.
I added two steps to call this command to run go build ./..., one for the main package and the other for the examples package.
The SDK version is already pined in the examples. Also I believe we setup the dependabot to check daily to detect a new version is released here.

@pgautier404
Copy link
Contributor

  • we should be verifying that examples build on every PR, BUT

    • they should be building against a pinned version, AND

This is what I was getting wrong (well, one of the things anyway), apologies. I had it in my head that the examples were being built against the latest commit, not the latest pinned version. You're right about that being the safe and standard way to do this. Sorry for the brain flatulence.

In talking about this yesterday with @poppoerika I tracked down the commit that caused us so many problems, and it was actually adding the examples into the linting step that caused us so many problems we had to revert it in 8618c44.

Copy link
Contributor

@pgautier404 pgautier404 left a comment

Choose a reason for hiding this comment

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

LGTM!

@poppoerika poppoerika merged commit c7de675 into main Mar 17, 2023
@poppoerika poppoerika deleted the chore/gh-workflow-update branch March 17, 2023 20:30
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.

check and ensure that dependabot is looking for new versions, and that PRs are building/testing examples
3 participants