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.

Sign up for GitHub

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

Jump to bottom

fix: remove redundant conditon for job in release pipeline #16355

Merged
merged 4 commits into from
Dec 7, 2021
Merged

fix: remove redundant conditon for job in release pipeline #16355

merged 4 commits into from
Dec 7, 2021

Conversation

tadelesh
Copy link
Member

@tadelesh tadelesh commented Dec 1, 2021

  • The purpose of this PR is explained in this or a referenced issue.
  • The PR does not update generated files.
  • Tests are included and/or updated for code changes.
  • Updates to CHANGELOG.md are included.
  • MIT license headers are included in each file.

@tadelesh tadelesh requested a review from benbp as a code owner December 1, 2021 03:17
@tadelesh tadelesh mentioned this pull request Dec 1, 2021
Merged
5 tasks
seankane-msft
seankane-msft approved these changes Dec 1, 2021
View reviewed changes
Copy link
Member

@seankane-msft seankane-msft left a comment

Choose a reason for hiding this comment

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

Can you verify this works by releasing aztemplate from this branch before merging?

weshaggard
weshaggard reviewed Dec 1, 2021
View reviewed changes
eng/pipelines/templates/jobs/archetype-go-release.yml
@@ -31,7 +31,6 @@ stages:
jobs:
- deployment: TagRepository
displayName: "Create release tag"
condition: and(succeeded(), eq(variables['NeedToRelease'], 'true'), ne(variables['Skip.TagRepository'], 'true'))
Copy link
Member

Choose a reason for hiding this comment

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

We only want to remove the "NeedToRelease" part of this condition.

Copy link
Member Author

Choose a reason for hiding this comment

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

Skip.TagRepository has already been checked with previous stage CheckRelease which is the dependency of this Release stage. So I removed it.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe I'm missing it but I don't see where Skip.TagRepository is checked. Can you point me to the place we check it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry. I've mixed the two conditions (Skip.Release, Skip.TagRepository) up 😟. I've fixed it.

@tadelesh
Copy link
Member Author

tadelesh commented Dec 2, 2021

Can you verify this works by releasing aztemplate from this branch before merging?

Release: https://dev.azure.com/azure-sdk/internal/_build/results?buildId=1224938&view=results
Skip Release: https://dev.azure.com/azure-sdk/internal/_build/results?buildId=1224955&view=results

@seankane-msft
Copy link
Member

Can you verify this works by releasing aztemplate from this branch before merging?

Release: https://dev.azure.com/azure-sdk/internal/_build/results?buildId=1224938&view=results

The Release pipeline failed on the Push changes step, that definition is in eng/common, @weshaggard is this a flaky issue or is there anything you see that caused this failure?

@weshaggard
Copy link
Member

That failure is because it was ran from @tadelesh's fork and test pipeline and not our official template pipeline. @tadelesh can you use our go - aztemplate (https://dev.azure.com/azure-sdk/internal/_build?definitionId=3058) to test? You might need to manually queue it and point it at refs/pull/16355/merge as the branch to test it because some things will not work in our release pipelines if triggered directly from a PR.

@tadelesh
Copy link
Member Author

tadelesh commented Dec 3, 2021 •
edited
Loading

That failure is because it was ran from @tadelesh's fork and test pipeline and not our official template pipeline. @tadelesh can you use our go - aztemplate (https://dev.azure.com/azure-sdk/internal/_build?definitionId=3058) to test? You might need to manually queue it and point it at refs/pull/16355/merge as the branch to test it because some things will not work in our release pipelines if triggered directly from a PR.

Thanks for your suggestion which also help me to know another way to test pipeline change under PR. Release result is here. Some validation failed for the create pull request for new version step. I don't know whether it is due to the merge ref as I haven't changed anything about the Update package version job. Skip release result is here. If there is no more serious problem with this PR, I want to merge it asap as it block the release of some mgmt. SDK.

@seankane-msft
Copy link
Member

That failure is because it was ran from @tadelesh's fork and test pipeline and not our official template pipeline. @tadelesh can you use our go - aztemplate (https://dev.azure.com/azure-sdk/internal/_build?definitionId=3058) to test? You might need to manually queue it and point it at refs/pull/16355/merge as the branch to test it because some things will not work in our release pipelines if triggered directly from a PR.

Thanks for your suggestion which also help me to know another way to test pipeline change under PR. Release result is here. Some validation failed for the create pull request for new version step. I don't know whether it is due to the merge ref as I haven't changed anything about the Update package version job. Skip release result is here. If there is no more serious problem with this PR, I want to merge it asap as it block the release of some mgmt. SDK.

@weshaggard is the failure on the Release result because it was released from a branch and not from the main branch? Once that issue is fixed, this looks good to me.

@seankane-msft seankane-msft mentioned this pull request Dec 3, 2021
Merged
@weshaggard
Copy link
Member

Yes the failure is because we cannot create a pull request against a PR merge that is expected.

@tadelesh once you handle my other feedback about the Skip.TagRepository this change looks good to me.

@weshaggard
Copy link
Member

@tadelesh while in here can you also address the warning I saw in the logs https://dev.azure.com/azure-sdk/internal/_build/results?buildId=1227441&view=logs&j=f4752989-c568-5678-80f2-0a67848bfa4d. You need to declare the machine pool and image in the CheckRelease stage similar to the other release stage otherwise it uses a default image.

@tadelesh
Copy link
Member Author

tadelesh commented Dec 4, 2021

@tadelesh while in here can you also address the warning I saw in the logs https://dev.azure.com/azure-sdk/internal/_build/results?buildId=1227441&view=logs&j=f4752989-c568-5678-80f2-0a67848bfa4d. You need to declare the machine pool and image in the CheckRelease stage similar to the other release stage otherwise it uses a default image.

Fixed. Test log is here.

@tadelesh tadelesh merged commit 28bebb2 into Azure:main Dec 7, 2021
@tadelesh tadelesh deleted the fix_prerelease_pipeline branch December 20, 2021 05:47
jhendrixMSFT pushed a commit to jhendrixMSFT/azure-sdk-for-go that referenced this pull request Jan 12, 2022
@tadelesh @jhendrixMSFT
fix: remove redundant conditon for job in release pipeline (Azure#16355)
67a98a9 
* fix: remove redundant conditon for job in release pipeline

* build: release aztemplate 0.1.0 to test release pipeline

* fix: revert condition of 'Skip.TagRepository'

* fix: add pool to 'check release' job to avoid lagging default image
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seankane-msft seankane-msft seankane-msft approved these changes

@weshaggard weshaggard weshaggard approved these changes

@benbp benbp Awaiting requested review from benbp benbp is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tadelesh @seankane-msft @weshaggard