Skip to content

fix: standardize ibm catalog json and da inputs #275

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

shemau
Copy link
Collaborator

@shemau shemau commented May 9, 2025

Description

Standardize solution inputs, including variables and catalog (tile) information.

Release required?

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

DA inputs change from resource_group_name and use_existing_resource_group to a single input, existing_resource_group_name. Input validations are made stricter.

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.

@shemau shemau requested a review from daniel-butler-irl as a code owner May 9, 2025 20:01
@shemau
Copy link
Collaborator Author

shemau commented May 9, 2025

/run pipeline

@shemau
Copy link
Collaborator Author

shemau commented May 9, 2025

/run pipeline

@shemau
Copy link
Collaborator Author

shemau commented May 9, 2025

The upgrade test(s) should fail due to the input name changes. They are not skipped yet, to validate the failure is as expected.

@shemau
Copy link
Collaborator Author

shemau commented May 10, 2025

/run pipeline

@shemau
Copy link
Collaborator Author

shemau commented May 10, 2025

The only error is a new property not existing on the main branch (as expected) for the upgrade tests

TestRunUpgradeExample 2025-05-10T11:20:54Z retry.go:99: Returning due to fatal error: FatalError{Underlying: error while running command: exit status 1; ╷
│ Error: Value for undeclared variable
│ 
│ A variable named "existing_resource_group_name" was assigned on the command
│ line, but the root module does not declare a variable of that name. To use
│ this value, add a "variable" block to the configuration.
╵}

@shemau
Copy link
Collaborator Author

shemau commented May 12, 2025

/run pipeline

@shemau
Copy link
Collaborator Author

shemau commented May 12, 2025

/run pipeline

@@ -73,7 +73,7 @@
}
],
"architecture": {
"descriptions": "Creates or uses an existing IBM Container Registry namespace, configures pull traffic and storage quotas, and supports upgrading the registry plan to Standard.",
"description": "Creates or uses an existing IBM Container Registry namespace, configures pull traffic and storage quotas, and supports upgrading the registry plan to Standard.",
Copy link
Member

Choose a reason for hiding this comment

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

We can remove the whole description from here. We got confirmation on Friday that this is a bug in their docs, and is not a valid field at all.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After discussion, this matches current schema. This should be fixed after the schema changes.

type = string
description = "The endpoint of the IBM Container Registry, eg. https://us.icr.io or https://de.icr.io, to change quotas"
Copy link
Member

Choose a reason for hiding this comment

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

The default does not have https:// is it optional? I think the default should match the description format to avoid confusion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed in latest commit

@shemau
Copy link
Collaborator Author

shemau commented May 12, 2025

/run pipeline

"key": "tags"
"key": "tags",
"custom_config": {
"grouping": "deployment",
Copy link
Member

Choose a reason for hiding this comment

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

one small tweak I think we should make across the board is add the type to the custom config. So before grouping, add "type": "array" @ocofaigh FYI
This way at least to me its clear what is happening. its an array type with string elements.
The groupings are unrelated to anything we do but are required.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

addressed in latest commit

@shemau
Copy link
Collaborator Author

shemau commented May 13, 2025

/run pipeline

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.

2 participants