feat: support for new quotas submodule and standard DA #208

Soaib024 · 2024-10-18T09:12:08Z

Description

issue: https://github.ibm.com/GoldenEye/issues/issues/11125

New submodule to set pull traffic and quota limit
Added new DA which can be used to create a namespace, upgarde to standard plan, set pull traffic and storage quota
(At a time only one namespace can be created, and plan and quotas can be set only for one registry this is done to avoid complex input variable (list of object))

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.

Soaib024 · 2024-10-18T10:34:44Z

Upgrade test is expected to fail as previous directory used by it no longer exists

daniel-butler-irl · 2024-10-21T12:44:00Z

modules/quotas/main.tf

+  destroy_method = "PATCH"
+  destroy_path   = "//${var.container_registry_endpoint}/api/v1/quotas"
+  destroy_data = jsonencode({
+    "storage_megabytes" : 500 # set to default 500 MB


This might be ok but my worry here is that if a user does not start at 500mb the destroy is not reverting to the previous value. Account settings with IAM settings have a similar problem. We should check how that module handles it. @ocofaigh this is what I mentioned

Here is what I was thinking of. The ibm_iam_account_settings resource supports setting the value NOT_SET for some account settings. And as per provider docs:

NOT_SET - to 'unset' a previous set value.

I don't know how the provider is doing this? Are the storing the previous value in the state?

We are using restapi provider here and there isn't any previous value in the state file, it's mostly the api response

Not sure what else we can do here. Do we have a provider issue open to support doing this in provider?

Created the provider issue -> IBM-Cloud/terraform-provider-ibm#5816

ocofaigh

Left some comments. Also the DA should support taking in an existing namespace and configuring it

ibm_catalog.json

ocofaigh · 2024-10-21T13:17:24Z

modules/quotas/main.tf

+  destroy_method = "PATCH"
+  destroy_path   = "//${var.container_registry_endpoint}/api/v1/quotas"
+  destroy_data = jsonencode({
+    "storage_megabytes" : 500 # set to default 500 MB


Here is what I was thinking of. The ibm_iam_account_settings resource supports setting the value NOT_SET for some account settings. And as per provider docs:

NOT_SET - to 'unset' a previous set value.

I don't know how the provider is doing this? Are the storing the previous value in the state?

modules/quotas/version.tf

ocofaigh · 2024-10-21T13:30:58Z

solutions/standard/variables.tf

+
+variable "region" {
+  type        = string
+  description = "The IBM Cloud region where the container registry namespace and retentation policy will be created."


It seems your only using region in the provider config. Meaning its going to apply to all resources in the DA. Since the namespace resources doesn't take in any region value, I suspect it takes it from the provider config? So what happens if a user want to create a global namespace? Does region support taking in the string global? If so, I suspect you will want to use a provider alias just for namespace creation.

Yes, using global as a region works

I have added a new provider block with namespace alias and also renamed region variable to namespace_region

This block will be used for resource group and to get a iam token

provider "ibm" { ibmcloud_api_key = var.ibmcloud_api_key region = "us-south" }

This block will be used to create namespace and set retentation policy

provider "ibm" { alias = "namespace" ibmcloud_api_key = var.ibmcloud_api_key region = var.namespace_region }

solutions/standard/variables.tf

ocofaigh · 2024-10-21T13:35:26Z

solutions/standard/variables.tf

+variable "container_registry_endpoint" {
+  description = "The endpoint of the ICR region, eg. https://us.icr.io or https://de.icr.io"
+  type        = string
+  default     = "us.icr.io"


why do we need to ask for this if we already ask for region? I suspect we can create a mapping between region and the corresponding CR endpoint.
We may though wan't to ask what endpoint type to use, and default to private

region field was used for namespace, whereas the registry endpoint field is used to set the plan and quotas for the registry. I have renamed region to namespace_region as it is more intuitive.

Additionally, there isn't a direct one-to-one mapping between regions and ICR endpoints. For instance, if I want to use the jp.icr.io registry, its local region name is ap-north, but it's also known as jp-tok.

I think directly inputting the endpoint with a clear description indicating that it will be used for updating plan and quotas would be a simpler and more convenient approach also we might need to maintain mapping and validation if theres any changes in future.

However, if you think a mapping would be beneficial, let me know, and I can create it based on this table https://cloud.ibm.com/docs/Registry?topic=Registry-registry_overview#registry_regions_local

I think if we can programatically determine this we should. I'm trying to reduce the amount of places where the consumer can make a mistake. If they entered jp-tok for namespace_region for example, they may not realise they also need to update this variable to be jp.icr.io. So I think its a better user experience if we do mapping. We do it in other DAs too (for example here)

one thing we may need to expose is an endpoint type incase user doesn't have access to public endpoint, and does have access to the IBM private one. But perhaps we can re-use the value passed with the provider_visibility input?

solutions/standard/variables.tf

solutions/standard/outputs.tf

ibm_catalog.json

Soaib024 · 2024-11-03T05:12:07Z

/run pipeline

ocofaigh

see latest comments

solutions/standard/variables.tf

ocofaigh · 2024-11-07T12:50:59Z

solutions/standard/variables.tf

+
+variable "storage_megabytes" {
+  type        = number
+  description = "The storage quota in megabytes for the container registry. Use -1 for unlimited storage."


What is the behaviour is no value is set? The description should include this information

ibm_catalog.json

solutions/standard/variables.tf

solutions/standard/main.tf

ocofaigh · 2024-11-07T13:33:09Z

modules/quotas/main.tf

+  destroy_method = "PATCH"
+  destroy_path   = "//${var.container_registry_endpoint}/api/v1/quotas"
+  destroy_data = jsonencode({
+    "storage_megabytes" : 500 # set to default 500 MB


Not sure what else we can do here. Do we have a provider issue open to support doing this in provider?

ibm_catalog.json

examples/complete/provider.tf

tests/pr_test.go

tests/other_test.go

ocofaigh · 2024-11-22T13:26:00Z

@Soaib024 what is the latest here?

…form-ibm-container-registry into add-da

iamar7 · 2024-11-25T15:05:29Z

/run pipeline

iamar7 · 2024-11-25T17:09:15Z

/run pipeline

iamar7 · 2024-11-25T17:13:08Z

/run pipeline

iamar7 · 2024-11-25T17:22:04Z

/run pipeline

iamar7 · 2024-11-25T17:28:41Z

/run pipeline

iamar7 · 2024-11-25T17:35:56Z

/run pipeline

iamar7 · 2024-11-25T21:46:52Z

/run pipeline

iamar7 · 2024-11-25T22:04:03Z

/run pipeline

terraform-ibm-modules-ops · 2024-11-29T16:43:22Z

🎉 This PR is included in version 1.3.0 🎉

The release is available on:

GitHub release
v1.3.0

Your semantic-release bot 📦🚀

Soaib024 added 7 commits October 16, 2024 21:28

support for da

013f47c

add reference diagram

a2669a1

add test

b0f689f

update release

f366ed3

update icon image

71fc794

update readme

78aad4b

update docs

2a2aac2

Soaib024 changed the title ~~[WIP] Add da~~ Support for new quotas submodule and standard DA Oct 18, 2024

Soaib024 added 2 commits October 18, 2024 15:49

fix cra scan

4e811e3

SKIP UPGRADE TEST

6dd3868

Soaib024 changed the title ~~Support for new quotas submodule and standard DA~~ feat: support for new quotas submodule and standard DA Oct 18, 2024

Soaib024 marked this pull request as ready for review October 18, 2024 10:48

Soaib024 requested review from shemau and daniel-butler-irl as code owners October 18, 2024 10:48

Soaib024 and others added 3 commits October 18, 2024 16:47

add catalog id

2b82a3c

update description

607a400

Merge branch 'main' into add-da

fb78118

daniel-butler-irl reviewed Oct 21, 2024

View reviewed changes

ocofaigh requested changes Oct 21, 2024

View reviewed changes

Soaib024 and others added 6 commits October 25, 2024 18:04

Merge branch 'main' into add-da

4cb6a52

address review comments

a22dce2

address review-comments

7b95ffe

address review comments

1e485f0

update diagram

d756ece

add provider alias

a1c3c1f

ocofaigh reviewed Oct 29, 2024

View reviewed changes

ibm_catalog.json Outdated Show resolved Hide resolved

Soaib024 and others added 3 commits October 29, 2024 17:44

support existing namespace

91c68d6

Merge branch 'main' into add-da

fbceddb

update test

7b17f59

Soaib024 requested a review from ocofaigh November 4, 2024 10:09

ocofaigh requested changes Nov 7, 2024

View reviewed changes

address review comments

ada1b29

ocofaigh reviewed Nov 7, 2024

View reviewed changes

tests/other_test.go Outdated Show resolved Hide resolved

Md Anam Raihan added 2 commits November 25, 2024 20:32

fix: update namespace module

6c6876f

Merge branch 'main' of https://github.com/terraform-ibm-modules/terra…

9db09d6

…form-ibm-container-registry into add-da

add existing resource test & update example

6a9c7f9

resolve pre-commit issues

60d2c9b

update cra config

1f888a0

remove typo

8b0520b

resolve pre-commit issues

cf2a0af

iamar7 self-assigned this Nov 25, 2024

resolve comments

e810475

update test

304a4d9

iamar7 requested a review from ocofaigh November 26, 2024 19:50

ocofaigh approved these changes Nov 29, 2024

View reviewed changes

ocofaigh merged commit bf49fb9 into main Nov 29, 2024
2 checks passed

ocofaigh deleted the add-da branch November 29, 2024 16:42

terraform-ibm-modules-ops added the released label Nov 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support for new quotas submodule and standard DA #208

feat: support for new quotas submodule and standard DA #208

Soaib024 commented Oct 18, 2024 •

edited

Loading

Soaib024 commented Oct 18, 2024

daniel-butler-irl Oct 21, 2024

ocofaigh Oct 21, 2024

Soaib024 Oct 29, 2024

ocofaigh Nov 7, 2024

iamar7 Nov 25, 2024

ocofaigh left a comment

ocofaigh Oct 21, 2024

ocofaigh Oct 21, 2024

Soaib024 Oct 29, 2024

ocofaigh Oct 21, 2024

Soaib024 Oct 29, 2024

ocofaigh Nov 7, 2024

ocofaigh Nov 7, 2024

Soaib024 commented Nov 3, 2024

ocofaigh left a comment

ocofaigh Nov 7, 2024

ocofaigh Nov 7, 2024

ocofaigh commented Nov 22, 2024

iamar7 commented Nov 25, 2024

iamar7 commented Nov 25, 2024

iamar7 commented Nov 25, 2024

iamar7 commented Nov 25, 2024

iamar7 commented Nov 25, 2024

iamar7 commented Nov 25, 2024

iamar7 commented Nov 25, 2024

iamar7 commented Nov 25, 2024

terraform-ibm-modules-ops commented Nov 29, 2024

feat: support for new quotas submodule and standard DA #208

feat: support for new quotas submodule and standard DA #208

Conversation

Soaib024 commented Oct 18, 2024 • edited Loading

Description

Release required?

Release notes content

Run the pipeline

Checklist for reviewers

For mergers

Soaib024 commented Oct 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ocofaigh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Soaib024 commented Nov 3, 2024

ocofaigh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ocofaigh commented Nov 22, 2024

iamar7 commented Nov 25, 2024

iamar7 commented Nov 25, 2024

iamar7 commented Nov 25, 2024

iamar7 commented Nov 25, 2024

iamar7 commented Nov 25, 2024

iamar7 commented Nov 25, 2024

iamar7 commented Nov 25, 2024

iamar7 commented Nov 25, 2024

terraform-ibm-modules-ops commented Nov 29, 2024

Soaib024 commented Oct 18, 2024 •

edited

Loading