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

fix: Add key vault to cognitive service - avm/res/cognitive-services/account #1932

Conversation

Agazoth
Copy link
Contributor

@Agazoth Agazoth commented May 14, 2024

Description

Add option for outputting keys to key vault

Pipeline Reference

Pipeline
avm.res.cognitive-services.account

Type of Change

  • Update to CI Environment or utlities (Non-module effecting changes)
  • Azure Verified Module updates:
    • Bugfix containing backwards compatible bug fixes, and I have NOT bumped the MAJOR or MINOR version in version.json:
      • Someone has opened a bug report issue, and I have included "Closes #{bug_report_issue_number}" in the PR description.
      • The bug was found by the module author, and no one has opened an issue to report it yet.
    • Feature update backwards compatible feature updates, and I have bumped the MINOR version in version.json.
    • Breaking changes and I have bumped the MAJOR version in version.json.
    • Update to documentation

Checklist

  • I'm sure there are no other open Pull Requests for the same update/change
  • I have run Set-AVMModule locally to generate the supporting module files.
  • My corresponding pipelines / checks run clean and green without any errors or warnings

@Agazoth Agazoth requested review from a team as code owners May 14, 2024 14:47
@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Triage 🔍 Maintainers need to triage still Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue labels May 14, 2024
Copy link
Contributor

@eriqua eriqua left a comment

Choose a reason for hiding this comment

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

Hey @Agazoth thanks for the active contribution, keep it up 💪
A general comment from my side:
Adding resource types which are not part of the primary resource type hierarchy of the module is usually borderline a resource module. In many cases, while discussing, we can realize that a pattern (ptn) module would be a better fit. Since this is the second PR with a similar approach, I'd suggest to open a GitHub issue explaining the intention of this addition, so that we can discuss there and identify for example:

  • which module type would be the best fit (res or ptn)
  • the best way to implement (naming, interface etc)
  • plan accordingly if needed to be adopted by other modules.

@ilhaan
Copy link
Member

ilhaan commented May 15, 2024

There is a discussion about the right approach for resources that emit secrets: #1934 - posting here for visibility

@matebarabas matebarabas changed the title fix: Add key vault to cognitive service fix: Add key vault to cognitive service - avm/res/cognitive-services/account May 21, 2024
@matebarabas matebarabas added Class: Resource Module 📦 This is a resource module and removed Needs: Triage 🔍 Maintainers need to triage still labels May 21, 2024
@ilhaan
Copy link
Member

ilhaan commented May 24, 2024

Discussion still ongoing #1934

@eriqua eriqua added the Status: Do Not Merge ⛔ Do not merge PRs with this label attached as they are not ready or aligned to future direction etc. label May 25, 2024
@eriqua eriqua added the Needs: Core Team 🧞 This item needs the AVM Core Team to review it label Jun 6, 2024
@Agazoth Agazoth requested a review from a team as a code owner August 14, 2024 04:49
@AlexanderSehr
Copy link
Contributor

Hey @Agazoth, you can ignore the failed job. It's due to a bug which is addressed by this PR #3038

@AlexanderSehr
Copy link
Contributor

Looks good form my side. @ilhaan / @jceval, any doubts?

Copy link
Contributor

@jceval jceval left a comment

Choose a reason for hiding this comment

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

No doubts. After reading through the discussion - I agree on all points from the arrived upon option and implementation.
@Agazoth PR looks good to me

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll merge the PR in once #3038 is merged as the pipeline will otherwise fail

@ChrisSidebotham / @jtracey93 / @eriqua a review would be much appreciated :)

@ilhaan
Copy link
Member

ilhaan commented Sep 6, 2024

#3038 has merged @AlexanderSehr

The pipeline badge is red @Agazoth

@AlexanderSehr
Copy link
Contributor

#3038 has merged @AlexanderSehr

The pipeline badge is red @Agazoth

@Agazoth, I believe it's an issue a Re-Run of the failed job should fix. That said, I'd also recommend to re-run the Set-AVMModule script once as there were some readme updates since this PR was opened.

@AlexanderSehr
Copy link
Contributor

AlexanderSehr commented Sep 7, 2024

@Agazoth,
because I just saw it happen in your fork - let me direct your attention to one very useful workflow you should run in your fork at least once (but it wouldn't hurt to do it from time to time again to also cover new modules): https://azure.github.io/Azure-Verified-Modules/contributing/bicep/bicep-contribution-flow/enable-or-disable-workflows/

All workflows have a trigger on main as that's what kicks the publishing off. As re-syncs with upstream are not unheard of, we created the linked workflow (you can find in your list of workflows in your fork) to disable all workflows. This has the benefit that you can enable individual workflows as you need them, while no others are triggered - ever again. 💪

@Agazoth
Copy link
Contributor Author

Agazoth commented Sep 7, 2024

@Agazoth, because I just saw it happen in your fork - let me direct your attention to one very useful workflow you should run in your fork at least once (but it wouldn't hurt to do it from time to time again to also cover new modules): https://azure.github.io/Azure-Verified-Modules/contributing/bicep/bicep-contribution-flow/enable-or-disable-workflows/

All workflows have a trigger on main as that's what kicks the publishing off. As re-syncs with upstream are not unheard of, we created the linked workflow (you can find in your list of workflows in your fork) to disable all workflows. This has the benefit that you can enable individual workflows as you need them, while no others are triggered - ever again. 💪

You are absolutely right, @AlexanderSehr and thank you for this! I have become pretty shavvy with the github cli now though 😄 - right now I am trying to convince Github actions to use a new subscription (I updated the Secrets with a new subscriptionid) but it is as if the old subscriptionid is stuck somewhere, and my google-fu seems to be weak today. There is no obvious setting that either cashes or force updates the secrets used in the workflow.

@AlexanderSehr
Copy link
Contributor

@Agazoth, because I just saw it happen in your fork - let me direct your attention to one very useful workflow you should run in your fork at least once (but it wouldn't hurt to do it from time to time again to also cover new modules): https://azure.github.io/Azure-Verified-Modules/contributing/bicep/bicep-contribution-flow/enable-or-disable-workflows/
All workflows have a trigger on main as that's what kicks the publishing off. As re-syncs with upstream are not unheard of, we created the linked workflow (you can find in your list of workflows in your fork) to disable all workflows. This has the benefit that you can enable individual workflows as you need them, while no others are triggered - ever again. 💪

You are absolutely right, @AlexanderSehr and thank you for this! I have become pretty shavvy with the github cli now though 😄 - right now I am trying to convince Github actions to use a new subscription (I updated the Secrets with a new subscriptionid) but it is as if the old subscriptionid is stuck somewhere, and my google-fu seems to be weak today. There is no obvious setting that either cashes or force updates the secrets used in the workflow.

That's indeed strange. Once you update the secrets, it should pick up on the new subscription immediately. :o

@Agazoth
Copy link
Contributor Author

Agazoth commented Sep 7, 2024

@Agazoth, because I just saw it happen in your fork - let me direct your attention to one very useful workflow you should run in your fork at least once (but it wouldn't hurt to do it from time to time again to also cover new modules): https://azure.github.io/Azure-Verified-Modules/contributing/bicep/bicep-contribution-flow/enable-or-disable-workflows/
All workflows have a trigger on main as that's what kicks the publishing off. As re-syncs with upstream are not unheard of, we created the linked workflow (you can find in your list of workflows in your fork) to disable all workflows. This has the benefit that you can enable individual workflows as you need them, while no others are triggered - ever again. 💪

You are absolutely right, @AlexanderSehr and thank you for this! I have become pretty shavvy with the github cli now though 😄 - right now I am trying to convince Github actions to use a new subscription (I updated the Secrets with a new subscriptionid) but it is as if the old subscriptionid is stuck somewhere, and my google-fu seems to be weak today. There is no obvious setting that either cashes or force updates the secrets used in the workflow.

That's indeed strange. Once you update the secrets, it should pick up on the new subscription immediately. :o

I think I have to file a bug report at Github. I have tried just about everything - including deleting - and the Action still knows the old subscriptionid. However much I wnat to close this PR now, it seems to have to wait until that wierd issue is solved. I'll follow up and update here once the issue is solved and the pipeline runs.

@AlexanderSehr
Copy link
Contributor

@Agazoth, because I just saw it happen in your fork - let me direct your attention to one very useful workflow you should run in your fork at least once (but it wouldn't hurt to do it from time to time again to also cover new modules): https://azure.github.io/Azure-Verified-Modules/contributing/bicep/bicep-contribution-flow/enable-or-disable-workflows/
All workflows have a trigger on main as that's what kicks the publishing off. As re-syncs with upstream are not unheard of, we created the linked workflow (you can find in your list of workflows in your fork) to disable all workflows. This has the benefit that you can enable individual workflows as you need them, while no others are triggered - ever again. 💪

You are absolutely right, @AlexanderSehr and thank you for this! I have become pretty shavvy with the github cli now though 😄 - right now I am trying to convince Github actions to use a new subscription (I updated the Secrets with a new subscriptionid) but it is as if the old subscriptionid is stuck somewhere, and my google-fu seems to be weak today. There is no obvious setting that either cashes or force updates the secrets used in the workflow.

That's indeed strange. Once you update the secrets, it should pick up on the new subscription immediately. :o

I think I have to file a bug report at Github. I have tried just about everything - including deleting - and the Action still knows the old subscriptionid. However much I wnat to close this PR now, it seems to have to wait until that wierd issue is solved. I'll follow up and update here once the issue is solved and the pipeline runs.

@Agazoth while you're try to resolve this - we could also take a detour and merge your PR into a branch in upstream from where I re-test it - and re-open it as a new PR. Git-history-wise you'd still be the author.

@Agazoth
Copy link
Contributor Author

Agazoth commented Sep 7, 2024

@Agazoth, because I just saw it happen in your fork - let me direct your attention to one very useful workflow you should run in your fork at least once (but it wouldn't hurt to do it from time to time again to also cover new modules): https://azure.github.io/Azure-Verified-Modules/contributing/bicep/bicep-contribution-flow/enable-or-disable-workflows/
All workflows have a trigger on main as that's what kicks the publishing off. As re-syncs with upstream are not unheard of, we created the linked workflow (you can find in your list of workflows in your fork) to disable all workflows. This has the benefit that you can enable individual workflows as you need them, while no others are triggered - ever again. 💪

You are absolutely right, @AlexanderSehr and thank you for this! I have become pretty shavvy with the github cli now though 😄 - right now I am trying to convince Github actions to use a new subscription (I updated the Secrets with a new subscriptionid) but it is as if the old subscriptionid is stuck somewhere, and my google-fu seems to be weak today. There is no obvious setting that either cashes or force updates the secrets used in the workflow.

That's indeed strange. Once you update the secrets, it should pick up on the new subscription immediately. :o

I think I have to file a bug report at Github. I have tried just about everything - including deleting - and the Action still knows the old subscriptionid. However much I wnat to close this PR now, it seems to have to wait until that wierd issue is solved. I'll follow up and update here once the issue is solved and the pipeline runs.

@Agazoth while you're try to resolve this - we could also take a detour and merge your PR into a branch in upstream from where I re-test it - and re-open it as a new PR. Git-history-wise you'd still be the author.

I would be perfectly fine with that 😃

@AlexanderSehr AlexanderSehr changed the base branch from main to users/alsehr/Agazoth/AddKVToCognitiveService September 7, 2024 13:44
@AlexanderSehr
Copy link
Contributor

Off we go then.

@AlexanderSehr AlexanderSehr merged commit eb86faf into Azure:users/alsehr/Agazoth/AddKVToCognitiveService Sep 7, 2024
5 checks passed
@Agazoth
Copy link
Contributor Author

Agazoth commented Sep 8, 2024

@AlexanderSehr I figured it out. I only updated the subscription in ARM_SUBSCRIPTION_ID without updating the AZURE_CREDENTIALS. RTFM is still a thing, I guess 😆 - maybe AZURE_CREDENTIALS should only contain secrets not already covered by other secrets?

@AlexanderSehr
Copy link
Contributor

AlexanderSehr commented Sep 8, 2024

Hey @Agazoth,
I'll do you one better - outside very view exceptions, we'll soon enable OIDC authentication thanks to @eriqua's work in this PR: #2792. With it you won't need a credential to begin wih 💪
That said, AZURE_CREDENTIALS is set up in the way it is because it's directly consumed by the login task in the pipeline. In other words, we don't have a task before the login to manipulate the value (and e.g., inject the subscription id).

@Agazoth
Copy link
Contributor Author

Agazoth commented Sep 8, 2024

That is awesome!

AlexanderSehr added a commit that referenced this pull request Sep 18, 2024
…account (#3222)

## Description

Follow-up to: #1932
cc: @Agazoth 

| Pipeline |
| -------- |

[![avm.res.cognitive-services.account](https://github.com/Azure/bicep-registry-modules/actions/workflows/avm.res.cognitive-services.account.yml/badge.svg?branch=users%2Falsehr%2FAgazoth%2FAddKVToCognitiveService&event=workflow_dispatch)](https://github.com/Azure/bicep-registry-modules/actions/workflows/avm.res.cognitive-services.account.yml)

## Type of Change

<!-- Use the check-boxes [x] on the options that are relevant. -->

- [ ] Update to CI Environment or utlities (Non-module effecting
changes)
- [x] Azure Verified Module updates:
- [x] Bugfix containing backwards compatible bug fixes, and I have NOT
bumped the MAJOR or MINOR version in `version.json`:
- [ ] Someone has opened a bug report issue, and I have included "Closes
#{bug_report_issue_number}" in the PR description.
- [ ] The bug was found by the module author, and no one has opened an
issue to report it yet.
- [ ] Feature update backwards compatible feature updates, and I have
bumped the MINOR version in `version.json`.
- [ ] Breaking changes and I have bumped the MAJOR version in
`version.json`.
  - [ ] Update to documentation

## Checklist

- [x] I'm sure there are no other open Pull Requests for the same
update/change
- [x] I have run `Set-AVMModule` locally to generate the supporting
module files.
- [x] My corresponding pipelines / checks run clean and green without
any errors or warnings

<!-- Please keep up to day with the contribution guide at
https://aka.ms/avm/contribute/bicep -->

---------

Co-authored-by: Axel B. Andersen <axel.boeg.andersen@atea.dk>
Co-authored-by: Javier Cevallos <javier.cevallos@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Class: Resource Module 📦 This is a resource module Needs: Core Team 🧞 This item needs the AVM Core Team to review it Status: Do Not Merge ⛔ Do not merge PRs with this label attached as they are not ready or aligned to future direction etc. Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants