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

Adding tags parameters with radapp defaults to Azure Recipes #60

Merged
merged 3 commits into from
Feb 21, 2024

Conversation

willvelida
Copy link
Contributor

Responding to #53, adding tags to Azure Recipes as parameters.

@willvelida willvelida requested a review from a team as a code owner February 9, 2024 00:58
@willvelida
Copy link
Contributor Author

@AaronCrawfis - If you have any feedback for me, let me know 🙂

'radapp.io/environment': context.environment.id
'radapp.io/application': context.application.id
'radapp.io/resource': context.resource.id
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@AaronCrawfis did you want these to be the names or ids? We used names here

For @willvelida the id will be a long string like /planes/radius/local/..../Applications.Core/applications/my-cool-app.

The name will be my-cool-app.

There's also a special-case to handle where the application can be null. You can see how we handled that here: https://github.com/radius-project/recipes/blob/main/local-dev/rabbitmqqueues.bicep#L60

Copy link
Contributor

Choose a reason for hiding this comment

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

did you want these to be the names or ids?

Since these are going to be used to identify an application within the Azure portal and not used for programmatic matching within a namespace, my preference is IDs. That way you can fully identity what's using this resource.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rynowak @AaronCrawfis I've kept the IDs in, but I've added null checking for the application. Hopefully this meets both requirements? 🙂

@@ -48,9 +48,17 @@ param skuTier string = 'Standard'
@description('ISO 8601 Default message timespan to live value. This is the duration after which the message expires, starting from when the message is sent to Service Bus. This is the default value used when TimeToLive is not set on a message itself.')
param defaultMessageTimeToLive string = 'P14D'

@description('The tags that will be applied to the resource')
param tags object = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Idk if we want to make this a raw parameter. In the current design if I wanted to set my own tags I'd be removing the radapp.io tags, even though that might not be my intention. Instead, you could instead use the union function to offer a tags parameter, and then union it with the set of radapp.io tags so you get both behaviors: https://learn.microsoft.com/en-us/azure/azure-resource-manager/bicep/bicep-functions-array#union

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the bicep files to change the tags to variables, add another parameter for tags that the user may want to provide and used the union function to apply them both.

…on application context in tag

Signed-off-by: Will Velida <[email protected]>
param tags object = {}

@description('The Radius specific tags that will be applied to the resource')
var radiusTags = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just attempted to deploy this Recipe and got this error message:

{
          "code": "RecipeDeploymentFailed",
          "message": "failed to deploy recipe default of type Applications.Dapr/stateStores",
          "details": [
            {
              "code": "DeploymentFailed",
              "message": "At least one resource deployment operation failed. Please see the details for the specific operation that failed.",
              "target": "/planes/radius/local/resourceGroups/default/providers/Microsoft.Resources/deployments/recipe1707868070557672174",
              "details": [
                {
                  "code": "InvalidTagNameCharacters",
                  "message": "The tag names 'radapp.io/application, radapp.io/environment, radapp.io/resource' have reserved characters '\u003c,\u003e,%,\u0026,\\,?,/' or control characters. These characters are only allowed for tags that start with the prefix 'hidden, link'.",
                  "target": "/subscriptions/66d1209e-1382-45d3-99bb-650e6bf63fc0/resourcegroups/aacrawfi/providers/Microsoft.Storage/storageAccounts/recipe2lkqwvspyl24g"
                }
              ]
            }

Looking in the Azure portal I also get the same error message:
image

We should switch to using another character, maybe -? (radapp.io-application)

cc/ @rynowak

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AaronCrawfis I've changed the character now to a hyphen :)

Question - Do the tests just work for the local recipes? If so, could we introduce tests to test the Azure/AWS recipes as part of the workflow? Initially I thought using ephemeral environments for each cloud to do this, but not 100% on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! Currently yes, we just test local-dev Recipes as we can run them in-cluster without needing external cloud resources. We do want to add full integration tests for cloud resources, we just haven't gotten it setup yet. I opened #62 to track this work.

Copy link
Contributor

@AaronCrawfis AaronCrawfis left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!!
image

@AaronCrawfis AaronCrawfis merged commit 6be2c36 into radius-project:main Feb 21, 2024
1 of 2 checks passed
@willvelida willvelida deleted the azure-tags branch February 21, 2024 21:46
Reshrahim pushed a commit to Reshrahim/recipes that referenced this pull request Feb 23, 2024
…project#60)

* Adding tags parameters with radapp defaults to Azure Recipes

Signed-off-by: Will Velida <[email protected]>

* Add radius-specific tags with optional tag parameters. null checking on application context in tag

Signed-off-by: Will Velida <[email protected]>

* Adding hyphen character in radiusTags object

Signed-off-by: Will Velida <[email protected]>

---------

Signed-off-by: Will Velida <[email protected]>
Signed-off-by: Reshma Abdul Rahim <[email protected]>
kachawla added a commit that referenced this pull request Feb 27, 2024
* Fix container name (#58)

Signed-off-by: Aaron Crawfis <[email protected]>
Signed-off-by: Reshma Abdul Rahim <[email protected]>

* Adding tags parameters with radapp defaults to Azure Recipes (#60)

* Adding tags parameters with radapp defaults to Azure Recipes

Signed-off-by: Will Velida <[email protected]>

* Add radius-specific tags with optional tag parameters. null checking on application context in tag

Signed-off-by: Will Velida <[email protected]>

* Adding hyphen character in radiusTags object

Signed-off-by: Will Velida <[email protected]>

---------

Signed-off-by: Will Velida <[email protected]>
Signed-off-by: Reshma Abdul Rahim <[email protected]>

* Add contribution guides

Signed-off-by: Reshma Abdul Rahim <[email protected]>

* Add contribution link

Signed-off-by: Reshma Abdul Rahim <[email protected]>

* Add contribution link

Signed-off-by: Reshma Abdul Rahim <[email protected]>

* Add links to contribution guides to code and PR

Signed-off-by: Reshma Abdul Rahim <[email protected]>

* Rename contribution folder

Signed-off-by: Reshma Abdul Rahim <[email protected]>

* Add links to contribution guides to code and PR

Signed-off-by: Reshma Abdul Rahim <[email protected]>

* Add testing steps

Signed-off-by: Reshma Abdul Rahim <[email protected]>

* Add testing steps

Signed-off-by: Reshma Abdul Rahim <[email protected]>

* close bicep code snippet

Signed-off-by: Reshma Abdul Rahim <[email protected]>

* Delete .DS_Store

Signed-off-by: Reshma Abdul Rahim <[email protected]>
Signed-off-by: Reshma Abdul Rahim <[email protected]>

* Address feedback

Signed-off-by: Reshma Abdul Rahim <[email protected]>

* Update docs/contributing/contributing-recipes.md

Co-authored-by: Aaron Crawfis <[email protected]>
Signed-off-by: Reshma Abdul Rahim <[email protected]>
Signed-off-by: Reshma Abdul Rahim <[email protected]>

* Address feedback

Signed-off-by: Reshma Abdul Rahim <[email protected]>

* Update publish in test workflow to exclude readme file)

Signed-off-by: Reshma Abdul Rahim <[email protected]>

* Empty-Commit

Signed-off-by: Reshma Abdul Rahim <[email protected]>

* Update aws/README.MD

Co-authored-by: Karishma Chawla <[email protected]>
Signed-off-by: Reshma Abdul Rahim <[email protected]>

* Apply suggestions from code review

Co-authored-by: Karishma Chawla <[email protected]>
Signed-off-by: Reshma Abdul Rahim <[email protected]>

* Address feedback

Signed-off-by: Reshma Abdul Rahim <[email protected]>

* Address feedback

Signed-off-by: Reshma Abdul Rahim <[email protected]>

* Address feedback

Signed-off-by: Reshma Abdul Rahim <[email protected]>

* Address feedback

Signed-off-by: Reshma Abdul Rahim <[email protected]>

* Add hyperlink

Signed-off-by: Reshma Abdul Rahim <[email protected]>

* Add hyperlink

Signed-off-by: Reshma Abdul Rahim <[email protected]>

---------

Signed-off-by: Aaron Crawfis <[email protected]>
Signed-off-by: Reshma Abdul Rahim <[email protected]>
Signed-off-by: Will Velida <[email protected]>
Signed-off-by: Reshma Abdul Rahim <[email protected]>
Co-authored-by: Aaron Crawfis <[email protected]>
Co-authored-by: Will Velida <[email protected]>
Co-authored-by: Karishma Chawla <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants