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

[Feature Request] BREAKING change: Rename DEPLOYMENT_SP_ID to DEPLOYMENT_SPN_ENTAPP_OBJID #1465

Closed
AlexanderSehr opened this issue Jun 2, 2022 · 5 comments · Fixed by #1752
Assignees
Labels
[cat] azure devops category: Azure DevOps [cat] github category: GitHub [cat] modules category: modules [cat] pipelines category: pipelines enhancement New feature or request
Milestone

Comments

@AlexanderSehr
Copy link
Contributor

AlexanderSehr commented Jun 2, 2022

Description

As discussed in #1016, we want to be more explicit with the name of the DEPLOYMENT_SP_ID variable in CARML. In the past there was some confusion around this value.

Note: This is a breaking change that will require downstream forks/clones to update their environment.

@AlexanderSehr AlexanderSehr added the enhancement New feature or request label Jun 2, 2022
@AlexanderSehr AlexanderSehr added [cat] pipelines category: pipelines [cat] modules category: modules [cat] github category: GitHub [cat] azure devops category: Azure DevOps labels Jun 2, 2022
@eriqua
Copy link
Contributor

eriqua commented Jun 5, 2022

I totally support this change and I understand the team already agreed on this name, just wondering while N in SPN stands for Name, wouldn't it be better to call it DEPLOYMENT_SP_ENTAPP_OBJID?

In addition, since as correctly stated this is a breaking change and given

I'd suggest to align on the name of all GH and ADO secrets in order not to introduce the same breaking change twice.
For example the OIDC PR should align with the name suggested by this actual feature request and similarly refer to DEPLOYMENT_SP_ENTAPP_APPID or DEPLOYMENT_SP_APPID instead of ARM_CLIENT_ID since we're talking about the same service principal.

@MariusStorhaug
Copy link
Contributor

@eriqua As a SP consist of an EntApp and AppReg, I would suggest just calling it DEPLOYMENT_SP_APPID in that case as AppID is shared between the two, aligning more to object/class oriented naming.
Will review these PRs tomorrow to align a bit more.

@MariusStorhaug
Copy link
Contributor

#1605 could make the rename obsolete.

@rahalan
Copy link
Contributor

rahalan commented Jul 7, 2022

@MariusStorhaug please elaborate on your findings with your PoC. Can we close the issue?

@eriqua eriqua changed the title [Feature Request]: Rename DEPLOYMENT_SP_ID to DEPLOYMENT_SPN_ENTAPP_OBJID [Feature Request] BREAKING change: Rename DEPLOYMENT_SP_ID to DEPLOYMENT_SPN_ENTAPP_OBJID Jul 25, 2022
@MariusStorhaug
Copy link
Contributor

@rahalan, I don't think we should close it until we agree if we want to automate the step? If we do then this issue is redundant and should not be worked on. I have not taken that issue myself yet, and removed it from the PR regarding OIDC as its combining a lot of overhead to the PR which wasn't the intention in the first place.

@AlexanderSehr AlexanderSehr self-assigned this Aug 3, 2022
@rahalan rahalan added blocked if an issue is blocked and removed [cat] needs further discussion labels Aug 4, 2022
@AlexanderSehr AlexanderSehr linked a pull request Aug 5, 2022 that will close this issue
4 tasks
@rahalan rahalan removed the blocked if an issue is blocked label Nov 10, 2022
@rahalan rahalan moved this to Done in Backlog Dec 11, 2022
@rahalan rahalan added this to Backlog Dec 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[cat] azure devops category: Azure DevOps [cat] github category: GitHub [cat] modules category: modules [cat] pipelines category: pipelines enhancement New feature or request
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants