Skip to content

feat: add order to coder_metadata #450

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

feat: add order to coder_metadata #450

wants to merge 6 commits into from
+38 −0

Conversation

@BrunoQuaresma
Copy link
Collaborator

Closes #325

@BrunoQuaresma BrunoQuaresma self-assigned this Oct 21, 2025
johnstcn
johnstcn reviewed Oct 21, 2025
View reviewed changes
provider/metadata.go Outdated
Type: schema.TypeInt,
Description: "The order determines the position of item in the UI presentation. The lowest order is shown first and items with equal order are sorted by key (ascending order).",
ForceNew: true,
Computed: true,
Copy link
Member

Choose a reason for hiding this comment

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

If you set Computed on an attribute, it cannot be changed. Ref: https://developer.hashicorp.com/terraform/plugin/sdkv2/schemas/schema-behaviors#computed

johnstcn
johnstcn requested changes Oct 22, 2025
View reviewed changes
Copy link
Member

@johnstcn johnstcn left a comment •
edited
Loading

Choose a reason for hiding this comment

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

You will need to update the tests and run make fmt / make gen in order for CI to pass.
My suggestion would also be to add/update tests in ./integration for this change.

@BrunoQuaresma
Copy link
Collaborator Author

BrunoQuaresma commented Oct 22, 2025 •
edited
Loading

@johnstcn I have updated the TestMetadata to include one item with the order attribute and added a check(you can check the "Files changed" tab), but it is not working 😞

Error:      	Not equal: 
								expected: "4"
								actual  : "0"

Do you have any idea why?

@BrunoQuaresma
Copy link
Collaborator Author

@johnstcn about adding an integration test for this, I think this change is simple enough to be tested only in the TestMetadata, but I would like to understand why you think an integration test for this would be necessary.

@johnstcn
Copy link
Member

I would like to understand why you think an integration test for this would be necessary.

Purely out of paranoia, but it's only a suggestion.

mtojek
mtojek reviewed Oct 23, 2025
View reviewed changes
provider/metadata_test.go Outdated
item {
key = "foo"
value = "bar"
order = 4
Copy link
Member

Choose a reason for hiding this comment

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

Can you add more test cases?

  1. All items have order present.
  2. Order is negative.
  3. Only a few items have order present

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we can have 1 and 3 on the same test. We can just add orders to all the items and leave one empty and check if it is set to 0(the expected default value).

About 2, I don't see we handling that in any other part of the provider for app or agent metadata order, but putting some thoughts on that, I think it should not be a problem since you can still sort things by negative numbers.

Copy link
Member

Choose a reason for hiding this comment

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

👍

provider/metadata.go
"order": {
Type: schema.TypeInt,
Description: "The order determines the position of item in the UI presentation. The lowest order is shown first and items with equal order are sorted by key (ascending order).",
ForceNew: true,
Copy link
Member

Choose a reason for hiding this comment

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

Hm... is ForceNew necessary @johnstcn ?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not 100% sure, but it is already used in agent.metadata.order so it may be no harm to be safe and stick to the status quo.

provider/metadata_test.go Outdated
"item.0.key": "foo",
"item.0.value": "bar",
"item.0.sensitive": "false",
"item.0.order": "4",
Copy link
Member

Choose a reason for hiding this comment

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

BTW this is not enough for metadata items. See the comment on top of this function.

PS. VS code debugger is very handy when dealing with Terraform schema.

@BrunoQuaresma
Copy link
Collaborator Author

@johnstcn related to the integration tests, what minVersion should I set when testing new features?

@johnstcn
Copy link
Member

johnstcn commented Oct 23, 2025 •
edited
Loading

@johnstcn related to the integration tests, what minVersion should I set when testing new features?

If you set minVersion: "v0.0.0" then the integration test will always run.
Otherwise, the test will only run based on the version of Coder against which the integration test is actually running.

In CI, the integration tests run against three different versions (from go run ./scripts/coderversion.go):

  • CODER_MAINLINE_VERSION: latest mainline version
  • CODER_STABLE_VERSION: latest stable version
  • CODER_OLDSTABLE_VERSION: previous stable version

So basically if a particular integration test only makes sense to run from a particular Coder version onwards, you set minVersion to that version.

johnstcn
johnstcn reviewed Oct 23, 2025
View reviewed changes
johnstcn
johnstcn reviewed Oct 23, 2025
View reviewed changes
provider/provider.go Outdated
return nil
}
var valueAsInt int64
gocty.FromCtyValue(value, &valueAsInt)
Copy link
Member

Choose a reason for hiding this comment

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

This can return an error, so we may to modify the return signature to (interface{}, error) and handle it in the calling function.

johnstcn
johnstcn reviewed Oct 24, 2025
View reviewed changes
provider/provider.go
Comment on lines +142 to +149
func valueAsInt(value cty.Value) (interface{}, error) {
if value.IsNull() {
return nil, nil
}
var valueAsInt int64
err := gocty.FromCtyValue(value, &valueAsInt)
return valueAsInt, err
}
Copy link
Member

Choose a reason for hiding this comment

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

With this change, it would be good to have a test for a non-numeric value for order.

mtojek
mtojek reviewed Oct 24, 2025
View reviewed changes
Copy link
Member

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

Once tests pointed our by Cian are 👍 , feel free to proceed.

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

Reviewers

@johnstcn johnstcn johnstcn requested changes

@mtojek mtojek mtojek left review comments

Requested changes must be addressed to merge this pull request.

Assignees

@BrunoQuaresma BrunoQuaresma

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add order to coder_metadata

3 participants

@BrunoQuaresma @johnstcn @mtojek