Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Task: Move ACR deployment to deploy stage #2285

Open
wants to merge 37 commits into
base: main
Choose a base branch
from

Conversation

Onokaev
Copy link
Contributor

@Onokaev Onokaev commented Mar 26, 2025

Completed fix, but holding until tomorrow to test end to end.
Deployment is blocked because of docker pull rate limits

Successful run: https://microsoftgraph.visualstudio.com/Graph%20Developer%20Experiences/_build/results?buildId=184590&view=logs&j=94deceea-7f99-5fa2-dcf4-7ec60dc7cc5a&t=94deceea-7f99-5fa2-dcf4-7ec60dc7cc5a

@Onokaev Onokaev marked this pull request as ready for review March 26, 2025 18:37
Write-Host "Found version in PropertyGroup array: $version"
break
- deployment: deploy_docker_image
environment: kiota-github-releases
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
environment: kiota-github-releases

We don't need this environment since we're not writing to github releases here

Copy link
Member

Choose a reason for hiding this comment

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

re-opening this since I saw that you added it back. Could you please provide some context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The job fails without an environment variable. I'll create a relevant one on ADO.
I put it back to test it

pool:
vmImage: 'ubuntu-latest'
steps:
- checkout: self
Copy link
Member

Choose a reason for hiding this comment

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

we might not be allowed to checkout the repository in a deployment stage, have you tested this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It actually works. Let me remove the checkout step

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, why is this not allowed?

Copy link
Member

Choose a reason for hiding this comment

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

deployment phases are not allowed to use checkout steps per security policy. Effectively they don't want scripts that could lead to some kind of injection I suppose.
If we revert to a regular job (not a deployment one) it'd be allowed though.
I have no doubt that it can work, but I'm fairly confident it'll trigger security items down the road.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it! Thanks

@Onokaev Onokaev marked this pull request as draft March 28, 2025 12:46
@Onokaev Onokaev marked this pull request as ready for review April 2, 2025 18:52
@Onokaev Onokaev requested a review from a team as a code owner April 2, 2025 18:52
@Onokaev Onokaev requested a review from baywet April 2, 2025 18:55
Copy link

sonarqubecloud bot commented Apr 2, 2025

displayName: 'Copy repository files for deploy stage'
inputs:
SourceFolder: '$(Build.SourcesDirectory)'
Contents: |
Copy link
Member

Choose a reason for hiding this comment

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

have you considered this option instead of copying the whole repo which is effectively working around the security constraints?
https://stackoverflow.com/questions/66468815/how-to-publish-docker-image-as-an-artifact-in-azure-devops

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants