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

feat: avm/res/app/container-app #833

Merged
merged 51 commits into from
Jan 29, 2024
Merged

Conversation

oZakari
Copy link
Contributor

@oZakari oZakari commented Jan 19, 2024

Description

Closes avm/res/app/container-app #128

If you haven't already, read the full contribution guide. The guide may have changed since the last time you read it, so please double-check. Once you are done and ready to submit your PR, edit the PR description and run through the relevant checklist below.

Enable GitHub Workflows in your fork to enable auto-generation of assets with our GitHub Action.
To trigger GitHub Actions after auto-generation, add a GitHub PAT as a secret in your forked repository called PAT.

edit: @AlexanderSehr

Pipeline
avm.res.app.container-app

Adding a new module

  • A proposal has been submitted and approved.
  • I have included "Closes #{module_proposal_issue_number}" in the PR description.
  • I have run brm validate locally to verify the module files.
  • I have run deployment tests locally to ensure the module is deployable.

@oZakari oZakari requested review from a team as code owners January 19, 2024 02:05
@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 Jan 19, 2024
@segraef segraef changed the title feat: avm/res/app/container-app feat: avm/res/app/container-app Jan 19, 2024
@segraef
Copy link
Contributor

segraef commented Jan 19, 2024

Please add a status check badge of your latest successful local e2e tests as proof. More details see here.

Copy link
Contributor

@AlexanderSehr AlexanderSehr left a comment

Choose a reason for hiding this comment

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

cc: @eriqua (I was so free to steal you comment from another PR)

Hi @oZakari, codeowners file should be updated. For guidelines on how to configure teams please refer to https://azure.github.io/Azure-Verified-Modules/specs/shared/#team-memberships

This refers to the .github/CODEOWNERS file. You should be able to find a corresponding line and can uncomment it :)

@oZakari oZakari force-pushed the container-apps branch 3 times, most recently from 6179391 to 96817eb Compare January 19, 2024 22:36
@oZakari
Copy link
Contributor Author

oZakari commented Jan 19, 2024

Hey @AlexanderSehr, any idea what I am missing with this test

Appears to be a couple of tests are failing:

  • Param category check: I checked the regex for the roleAssigments param and it looks fine. image
  • Telemetry identifier - The code looks correct from my perspective, so not sure what I am missing with this one.

@AlexanderSehr
Copy link
Contributor

Hey @AlexanderSehr, any idea what I am missing with this test

Appears to be a couple of tests are failing:

  • Param category check: I checked the regex for the roleAssigments param and it looks fine. image
  • Telemetry identifier - The code looks correct from my perspective, so not sure what I am missing with this one.

Hey @oZakari,

thanks for working through the comments. Regarding the failing tests I just checked your pipeline and would recommend to re-run it. The commit the pipeline uses is not the latest you actually pushed.
As a result e.g. the roleAssignement.condition parameter in the pipeline run is actually missing a . at the end, which is already fixed in your latest version and hence should not lead to any failing test.

Regarding the telemetry, it is actually expecting a slightly different value, as you can see in the following:

  • Expected: 46d3xbcp.res.app-containerapp
  • Current: 46d3xbcp.res.app-container-app
    For reference: The value is taken from the CSV linked here.
    Again, I can see this was fixed in the latest commit, so please make sure the readme & json are the latest using the Set-avmmodule -ModuleFolderPath '.\avm\res\app\container-app' -Recurse utility and re-run the pipeline :)

@oZakari
Copy link
Contributor Author

oZakari commented Jan 23, 2024

Hi @AlexanderSehr, I think I got everything sorted at this point. With your help of course, so thanks much! Let me know if you need anything else from me.

@matebarabas matebarabas added Class: Resource Module 📦 This is a resource module and removed Needs: Triage 🔍 Maintainers need to triage still labels Jan 23, 2024
@AlexanderSehr
Copy link
Contributor

Hi @AlexanderSehr, I think I got everything sorted at this point. With your help of course, so thanks much! Let me know if you need anything else from me.

Sad, but interesting. One would wish the RP would at least be so kind to throw an error. Meh

@AlexanderSehr AlexanderSehr merged commit 4d243a1 into Azure:main Jan 29, 2024
9 checks passed
@oZakari oZakari deleted the container-apps branch January 29, 2024 19:18
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 Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants