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

PR resource names include build number #1509

Merged
merged 32 commits into from
Dec 15, 2020
Merged

Conversation

LTA-Thinking
Copy link
Collaborator

Description

Changes the names of the resources created for testing a PR to include the build number in addition to the PR number.

Related issues

Previously we have encountered an error where resource names were conflicting with existing resources. This would require a new PR to be created as the names of the resources was based only on the PR number. This change will change the names of the resources on each build, allowing a rerun of the build to fix this issue.

Testing

Running the PR.

FHIR Team Checklist

  • Update the title of the PR to be succinct and less than 50 characters
  • Add a milestone to the PR for the sprint that it is merged (i.e. add S47)
  • Tag the PR with the type of update: Bug, Enhancement, or New-Feature
  • Tag the PR with Azure API for FHIR if this will release to the managed service
  • Review squash-merge requirements

@LTA-Thinking LTA-Thinking requested a review from a team as a code owner December 10, 2020 19:09
}
catch
{}
New-AzResourceGroup -Name "$(ResourceGroupName)" -Location "$(ResourceGroupRegion)" -Force
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Dec 10, 2020

Choose a reason for hiding this comment

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

New-AzResourceGroup [](start = 10, length = 19)

Aren't this operation idempotent? Why we want drop resource group and everything in it? #Closed

Copy link
Collaborator Author

@LTA-Thinking LTA-Thinking Dec 10, 2020

Choose a reason for hiding this comment

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

If the previous build didn't successfully complete the resource group from the last build will still be around. Before this change the old resources would be overwritten by the new ones since they would have the same name. Now new resources with different names will be added. To avoid pilling up a lot of resources in the resource group I added this to clean it up before the new resources are made. #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

Make sense, on the same note, what happens to resources if someone closes PR for failed build? Are they automatically cleaned?


In reply to: 540447194 [](ancestors = 540447194)

Copy link
Member

@brendankowitz brendankowitz Dec 10, 2020

Choose a reason for hiding this comment

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

The build number is different for a few reasons, e.g.
If I have 1.0.1-pr1.1
I add a commit to fix my build => 1.0.1-pr1.2
Someone checks into master then I add a commit => 1.0.2-pr1.2

Copy link
Collaborator Author

@LTA-Thinking LTA-Thinking Dec 11, 2020

Choose a reason for hiding this comment

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

If someone closes a PR with a failed build the resource group is not cleaned up. This is an open issue for us. I had added a script to cleanup a resource group when a PR was closed, but it wasn't working because the GithubAction to run AzureDevOps pipelines doesn't have a feature we need (Azure/pipelines#17) #Closed

@@ -3,7 +3,8 @@ variables:
resourceGroupRoot: 'msh-fhir-pr'
appServicePlanName: '$(resourceGroupRoot)-$(prNumber)-asp'
prNumber: $(system.pullRequest.pullRequestNumber)
DeploymentEnvironmentName: '$(resourceGroupRoot)-$(prNumber)'
ResourceGroupName: '$(resourceGroupRoot)-$(prNumber)'
DeploymentEnvironmentName: '$(build.BuildNumber)'
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Dec 10, 2020

Choose a reason for hiding this comment

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

. [](start = 39, length = 1)

looks like . is not allowed for keyvault name #Closed

Copy link
Member

Choose a reason for hiding this comment

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

One of the other variables replaces them with dashes, I think it was $(GitVersion.NuGetVersionV2)
For reference: https://github.com/GitTools/actions/blob/master/gitversion/execute/action.yml

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is super helpful! Thank you!

Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka left a comment

Choose a reason for hiding this comment

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

:shipit:

@brendankowitz brendankowitz added the Area-Deployment Area related to deployment. label Dec 10, 2020
@LTA-Thinking
Copy link
Collaborator Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -3,7 +3,8 @@ variables:
resourceGroupRoot: 'msh-fhir-pr'
appServicePlanName: '$(resourceGroupRoot)-$(prNumber)-asp'
prNumber: $(system.pullRequest.pullRequestNumber)
DeploymentEnvironmentName: '$(resourceGroupRoot)-$(prNumber)'
ResourceGroupName: '$(resourceGroupRoot)-$(prNumber)'
DeploymentEnvironmentName: 'f$(build.BuildNumber)'
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Dec 14, 2020

Choose a reason for hiding this comment

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

f [](start = 32, length = 1)

why plain f is better than fhir? #Closed

Copy link
Collaborator Author

@LTA-Thinking LTA-Thinking Dec 14, 2020

Choose a reason for hiding this comment

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

It is shorter. I'm running into issues with the full build number being too long for some resource types. There is a max name length of 24 characters. #Closed

@LTA-Thinking LTA-Thinking merged commit 073e656 into master Dec 15, 2020
@LTA-Thinking LTA-Thinking deleted the personal/rojo/guid-conflict branch December 15, 2020 23:20
@LTA-Thinking LTA-Thinking added this to the S52 milestone Dec 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Deployment Area related to deployment.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants