Skip to content

Conversation

@dishankkalra23
Copy link
Member

@dishankkalra23 dishankkalra23 commented Oct 8, 2025

Description

This PR adds Service to Service Authorisation DA in existing module

Issue

#281

Release required?

  • No release
  • Patch release (x.x.X)
  • Minor release (x.X.x)
  • Major release (X.x.x)
Release notes content

Run the pipeline

If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.

Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:

/run pipeline

Checklist for reviewers

  • If relevant, a test for the change is included or updated with this PR.
  • If relevant, documentation for the change is included or updated with this PR.

For mergers

  • Use a conventional commit message to set the release level. Follow the guidelines.
  • Include information that users need to know about the PR in the commit message. The commit message becomes part of the GitHub release notes.
  • Use the Squash and merge option.

@dishankkalra23 dishankkalra23 marked this pull request as ready for review October 9, 2025 02:34
@dishankkalra23 dishankkalra23 self-assigned this Oct 13, 2025
@dishankkalra23 dishankkalra23 added the ready-for-review PR is ready for review label Oct 13, 2025
@dishankkalra23
Copy link
Member Author

/run pipeline

@dishankkalra23
Copy link
Member Author

/run pipeline

@toddgiguere
Copy link
Member

Upgrade test failed which is expected, this PR is already a known breaking change. Due to the renaming of the terraform resources from an indexed count (integer) to a map key (string), it is causing upgrade test to destroy and recreate all policies:

module.service_auth_cbr_rules.ibm_iam_authorization_policy.auth_policies[0] will be destroyed
module.service_auth_cbr_rules.ibm_iam_authorization_policy.auth_policies[1] will be destroyed

module.service_auth_cbr_rules.ibm_iam_authorization_policy.auth_policies["test-policy-1"] will be created
module.service_auth_cbr_rules.ibm_iam_authorization_policy.auth_policies["test-policy-2"] will be created

We will skip upgrade test from this point on.

this change has a known breaking change, skipping any upgrade tests

SKIP UPGRADE TEST
@toddgiguere
Copy link
Member

/run pipeline

toddgiguere
toddgiguere previously approved these changes Oct 16, 2025
@toddgiguere
Copy link
Member

/run pipeline

Copy link
Contributor

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

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

You need to onboard this to the catalog pipeline..

variable "prefix" {
type = string
nullable = true
description = "The prefix to be added to all resources created by this solution. To skip using a prefix, set this value to null or an empty string. The prefix must begin with a lowercase letter and may contain only lowercase letters, digits, and hyphens '-'. It should not exceed 16 characters, must not end with a hyphen('-'), and can not contain consecutive hyphens ('--'). Example: prod-cbr. [Learn more](https://terraform-ibm-modules.github.io/documentation/#/prefix.md)."
Copy link
Contributor

Choose a reason for hiding this comment

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

please follow the guidance here for prefix input

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated var.prefix

Copy link
Member Author

Choose a reason for hiding this comment

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

@ocofaigh Regarding the prefix variable, it is safe to remove prefix from the DA as it is not used in the creation of the auth policy. It was only referenced in cbr_rules here which we have already disabled by default here.

ibm_catalog.json Outdated
},
{
"key": "service_map",
"required": true
Copy link
Contributor

Choose a reason for hiding this comment

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

please use the HCL editor for this

Copy link
Member Author

Choose a reason for hiding this comment

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

@ocofaigh what's the type for hcl editor? is it "code_editor"?

default = null
}

variable "enable_cbr" {
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to expose this in the example as a variable. Just set it to false in the main.tf of the example. We don't want examples to become flexible solutions

Co-authored-by: Conall Ó Cofaigh <[email protected]>
@dishankkalra23
Copy link
Member Author

/run pipeline

@dishankkalra23
Copy link
Member Author

/run pipeline

##############################################################################

variable "service_map" {
description = "Map of unique service pairs and their authorization config."
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a complex object - consumers of the DA will not know the required format. A helper markdown doc is required that this description should link to. For example -> https://github.com/terraform-ibm-modules/terraform-ibm-kms-all-inclusive/blob/main/solutions/fully-configurable/DA-keys.md

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we get an initial release of the DA and later I can add the helper doc after coming from leave next week?

Copy link
Contributor

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

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

Left another comment - please stop triggering pipeline until it has been approved

@dishankkalra23
Copy link
Member Author

/run pipeline

@dishankkalra23
Copy link
Member Author

Left another comment - please stop triggering pipeline until it has been approved

Just saw the comment after triggering it.

@ocofaigh ocofaigh merged commit 0daf93b into main Oct 17, 2025
2 checks passed
@ocofaigh ocofaigh deleted the issue-281 branch October 17, 2025 17:54
@terraform-ibm-modules-ops
Copy link
Contributor

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-review PR is ready for review released

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants