-
Notifications
You must be signed in to change notification settings - Fork 774
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
feat: Add build_type to github_repository (pages) #1663
feat: Add build_type to github_repository (pages) #1663
Conversation
da8683f
to
e9f9867
Compare
e9f9867
to
0c0f7ef
Compare
Looking forward to having this live |
@0x46616c6b to validate the field you could do the checks here Validation functions are only supported for basic types, so the only option I have found is to do the checks on creation and update and in this case creation reuses update, so only update would work fine I guess. Maybe @kfcampbell could clarify if that is the case and if this is needed for the PR to get merged |
* `source` - (Required) The source branch and directory for the rendered Pages site. See [GitHub Pages Source](#github-pages-source) below for details. | ||
* `source` - (Optional) The source branch and directory for the rendered Pages site. See [GitHub Pages Source](#github-pages-source) below for details. | ||
|
||
* `build_type` - (Optional) The type of GitHub Pages site to build. Can be `legacy` or `workflow`. If you use `legacy` as build type you need to set the option `source`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if both source and build_type are present, which one takes precedence? What happens if legacy is set for build_type but source isn't set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest I am not aware what the GitHub API will return when the options are not set correctly. I can only outline the possible usage. Would like to prevent this with the Terraform validation but not sure if this is possible.
Allowed Options
resource "github_repository" "example" {
name = "example"
pages {
source {
branch = "main"
path = "/"
}
build_type = "legacy"
}
}
resource "github_repository" "example" {
name = "example"
pages {
source {
branch = "main"
path = "/"
}
}
}
resource "github_repository" "example" {
name = "example"
pages {
build_type = "workflow"
}
}
Problematic / wrong Configuration
resource "github_repository" "example" {
name = "example"
pages {
source {
branch = "main"
path = "/"
}
build_type = "workflow"
}
}
I'm okay with the validation approach; not running checks on creation/update should result in an error at the API level that will be passed back to the user in any case. @0x46616c6b does this result in a breaking change for users? I'm not sure if providing "legacy" by default on all build_types results in any API-level changes or not. |
I understand that upgrading the provider and applying the changes will cause a difference (in state only), but it won't affect the repository configuration. If this change is considered a breaking change (due to the new key changing the state), we should reflect that too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay with state changing as a non-breaking change as long as the API/GitHub state remains consistent. Thanks!
PR integrations#1663 added the "build_type" attribute to the github_repository resource. Since the corresponding data source uses the same code to parse the pages endpoint data, any use of the github_repository datasource would lead to an error, since the attribute did not exist there: > Error: error setting pages: Invalid address to set: []string{"pages", "0", "build_type"} This commit adds the missing attribute. The existing acceptance tests already cover it, failed for me when running without this commit, and passed after.
This appears to have broken some use cases: #1724 |
PR #1663 added the "build_type" attribute to the github_repository resource. Since the corresponding data source uses the same code to parse the pages endpoint data, any use of the github_repository datasource would lead to an error, since the attribute did not exist there: > Error: error setting pages: Invalid address to set: []string{"pages", "0", "build_type"} This commit adds the missing attribute. The existing acceptance tests already cover it, failed for me when running without this commit, and passed after. Co-authored-by: Keegan Campbell <me@kfcampbell.com>
Co-authored-by: Keegan Campbell <me@kfcampbell.com>
…1710) PR integrations#1663 added the "build_type" attribute to the github_repository resource. Since the corresponding data source uses the same code to parse the pages endpoint data, any use of the github_repository datasource would lead to an error, since the attribute did not exist there: > Error: error setting pages: Invalid address to set: []string{"pages", "0", "build_type"} This commit adds the missing attribute. The existing acceptance tests already cover it, failed for me when running without this commit, and passed after. Co-authored-by: Keegan Campbell <me@kfcampbell.com>
Resolves #1267
Behavior
Before the change?
After the change?
build_type
option is added topages
propertyAdditional info
Pull request checklist
Does this introduce a breaking change?
Please see our docs on breaking changes to help!
Type: Breaking change
label)Open Questions
build_type = "legacy"
thatsource
is not optional anymore?