Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Automate SDK release from CI #2457
Automate SDK release from CI #2457
Changes from 35 commits
dbf7400
e6b55cc
e21f709
e186ef7
da17089
e936664
4bf8118
33e6b21
d7825bd
38c1e5d
0d0010e
9848ff9
9a9cd6e
3571f63
a3fc1b6
ea2214b
3598d87
0919697
652fb51
b296c37
8ce7465
e1090d8
8a02492
c12133c
9a2f219
491df05
d80346b
6e306c7
b327cc7
8c0a3fd
36b5358
17b42c3
a3d9617
6c812b7
d4e1607
357b341
816fc7b
fb552e5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes the behavior of checkout@v2 to checkout the HEAD rather than the merge commit - this is useful in regular PRs, but especially important for releases, where we don't want to build the merged thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to double check that I got this right:
Our main workflow is triggered with push on master/main and on pull_request (with events opened, synchronize, or reopened). Since
ref
defaults to the reference or SHA of the event that triggered the workflow, you're saying that we need to avoid to fetch a push to master, which generally happens to be a merge of a pull request, but we always need to carefully fetch the head of the current PR which could be lacking the latest merged PRs that master has, which is a good thing in this case.Did I get this right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default behavior for checkout on a PR trigger is to checkout the merge commit (i.e. have the PR branch merged into the base branch and use that). This is why when there are merge conflicts, our GHA workflows don't run. This is the change suggested in the checkout action docs to make sure that PR builds actually build and test the code that is in the PR itself.
While for PR builds against master, checking out the merge commit is not a big deal - and in some cases may even be useful - for release PRs it's plain wrong and may result in very surprising behavior where we actually push a nuget package containing code that was not present in the release branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now it all makes sense. I wasn't fully aware of the default behaviour for the checkout on PRs, that's why I got confused. Thank you for the explanation. On top of this, the change of line 74 will stop "there are merge conflicts with master" that prevent the workflow to run; which I'm actually rather happy about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the important change that fixed the docs issues - moving it to after the package was built allows docfx to pick up the correct references.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦🏻.... makes sense