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

Allow full_path in addition to id in gitlab_project data source #532

Merged
merged 1 commit into from Jan 28, 2021
Merged

Allow full_path in addition to id in gitlab_project data source #532

merged 1 commit into from Jan 28, 2021

Conversation

sirlatrom
Copy link
Contributor

Fixes #140.

Copy link
Collaborator

@armsnyder armsnyder left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. A better fix would be to change the type for id into string in the resource schema (this is a backward compatible change) and update the documentation to mention that id can be an int or string. I don't think full_path is the standard name for this field in the GitLab API, nor is it used anywhere else in this plugin.

@sirlatrom
Copy link
Contributor Author

Thanks for the contribution. A better fix would be to change the type for id into string in the resource schema (this is a backward compatible change) and update the documentation to mention that id can be an int or string. I don't think full_path is the standard name for this field in the GitLab API, nor is it used anywhere else in this plugin.

It's used here:

"full_path": {
Type: schema.TypeString,
Computed: true,
Optional: true,
ConflictsWith: []string{
"group_id",
},
},
which is what I took as inspiration.

@armsnyder
Copy link
Collaborator

It's used here:

"full_path": {
Type: schema.TypeString,
Computed: true,
Optional: true,
ConflictsWith: []string{
"group_id",
},
},

which is what I took as inspiration.

Ahh, good point. However, I would still rather stick to the official GitLab API field names, as I see this as API debt.

See the API fields names for GET Project: https://docs.gitlab.com/ee/api/projects.html#get-single-project

See the HashiCorp guidance on this subject: https://www.terraform.io/docs/extend/hashicorp-provider-design-principles.html#resource-and-attribute-schema-should-closely-match-the-underlying-api

@sirlatrom
Copy link
Contributor Author

Ahh, good point. However, I would still rather stick to the official GitLab API field names, as I see this as API debt.

See the API fields names for GET Project: https://docs.gitlab.com/ee/api/projects.html#get-single-project

See the HashiCorp guidance on this subject: https://www.terraform.io/docs/extend/hashicorp-provider-design-principles.html#resource-and-attribute-schema-should-closely-match-the-underlying-api

OK, I'll change the type of id, but are you quite sure you can go from a TypeInt to TypeString without state parsing issues? I'll give it a try.

@armsnyder
Copy link
Collaborator

OK, I'll change the type of id, but are you quite sure you can go from a TypeInt to TypeString without state parsing issues? I'll give it a try.

Yep, going from int -> string is fine. Terraform will update the type in the state on the next apply, and users can even continue to set this field to a number in their config.

@sirlatrom
Copy link
Contributor Author

Yep, going from int -> string is fine. Terraform will update the type in the state on the next apply, and users can even continue to set this field to a number in their config.

@armsnyder Done in 55a5f54.

Copy link
Collaborator

@armsnyder armsnyder left a comment

Choose a reason for hiding this comment

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

Great! One final note.

docs/data-sources/project.md Outdated Show resolved Hide resolved
Fixes #140.

Signed-off-by: Sune Keller <absukl@almbrand.dk>
Copy link
Collaborator

@armsnyder armsnyder left a comment

Choose a reason for hiding this comment

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

Thanks!

@armsnyder armsnyder merged commit 0c64a2b into gitlabhq:master Jan 28, 2021
@sirlatrom sirlatrom deleted the fix-140 branch January 29, 2021 12:48
@github-actions github-actions bot locked and limited conversation to collaborators Nov 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
2 participants