Correct documentation of provider configuration, and deprecate organization argument #735
+63
−110
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Remove 'Organization' field, which was never read - Remove 'Individual' field, which was never read - Replace 'Anonymous' field with a function (as the field is derived from the value of 'Token')
We don't need `organization` and `GITHUB_ORGANIZTION`, because the current behaviour is that the user can put a user name _or_ an organization name in any of `owner`, `organization`, `GITHUB_OWNER`, or `GITHUB_ORGANIZATION`. It therefore seems clearer to just keep `owner` and `GITHUB_OWNER`. Unfortunately, the current precedence of owner, GITHUB_OWNER, organization, GITHUB_ORGANIZATION is also a bit confusing. From high precedence to low precedence, the order is: - organization - GITHUB_ORGANIZATION - GITHUB_OWNER - owner For backwards compatibility, in minor/patch releases we need to preserve this order. However, I've contained the code that does this in a single clearly marked block of code in providerConfigure so that it can be removed easily in a future major release. I've updated the documentation with: - a description of the current behaviour - the fact that organization and GITHUB_ORGANIZATION are deprecated - a warning that the precedence of owners/GITHUB_OWNER may change
majormoses
approved these changes
Mar 23, 2021
There was a problem hiding this comment.
Not much of a go developer, LGTM. It's a good solution to a bad problem, I would ideally like to see this release go out and then decide when we are willing to force the breaking change. I honestly don't see a big reason to wait long, if we have breaking changes that affect a small area it's much easier to review changelogs and upgrade then it is with large releases with multiple breaking changes.
jcudit
approved these changes
Apr 6, 2021
There was a problem hiding this comment.
I think this is a necessary change to get into the project to clean up from past mistakes. A huge thank you @tibbes for such a readable fix. I feel this will remove more frustration than it causes, so lets ship and track the removal of the backwards compatibility for the next major release in the coming months.
Changes to this part of the code base have historically not gone smoothly, so I'll be keeping an eye out for related bugs after this is released. Thanks to anyone who can quickly point out fixes as we upgrade this area. 🚀
ghost
mentioned this pull request
Closed
kfcampbell
pushed a commit
to kfcampbell/terraform-provider-github
that referenced
this pull request
Jul 26, 2022
…zation argument (integrations#735) * Remove dead function * Remove redundant call to os.Getenv * Simplify github.Config struct - Remove 'Organization' field, which was never read - Remove 'Individual' field, which was never read - Replace 'Anonymous' field with a function (as the field is derived from the value of 'Token') * Deprecate organization and GITHUB_ORGANIZATION We don't need `organization` and `GITHUB_ORGANIZTION`, because the current behaviour is that the user can put a user name _or_ an organization name in any of `owner`, `organization`, `GITHUB_OWNER`, or `GITHUB_ORGANIZATION`. It therefore seems clearer to just keep `owner` and `GITHUB_OWNER`. Unfortunately, the current precedence of owner, GITHUB_OWNER, organization, GITHUB_ORGANIZATION is also a bit confusing. From high precedence to low precedence, the order is: - organization - GITHUB_ORGANIZATION - GITHUB_OWNER - owner For backwards compatibility, in minor/patch releases we need to preserve this order. However, I've contained the code that does this in a single clearly marked block of code in providerConfigure so that it can be removed easily in a future major release. I've updated the documentation with: - a description of the current behaviour - the fact that organization and GITHUB_ORGANIZATION are deprecated - a warning that the precedence of owners/GITHUB_OWNER may change
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While investigating #662, I found that the code handling the
ownerandorganizationarguments to the GitHub provider could be simplified, and also that the documentation was incorrect.Although the docs say that
owneris for user ids (liketorvalds) andorganizationis for organization ids (likegithub), in fact, either type of value can be put in either argument.Given this, I suggest that we deprecate
organization, and just useowner.The main change in this pull request is to make
organizationandGITHUB_ORGANIZATIONdeprecated. Making arguments deprecated should only me done as part of a minor (or higher) release, according to the Hashicorp versioning guidelines.In addition, the documentation says that
ownercan be set via the environment variableGITHUB_OWNER, andorganizationcan be set via the environment variableGITHUB_ORGANIZATION. However, in reality, both environment variables (withGITHUB_ORGANIZATIONtaking priority) were used as the default value of bothownerandorganization. This had the odd side-effect of making the precedence of these methods inconsistent. From high to low precedence, the order is:organizationin the provider configurationGITHUB_ORGANIZATIONenvironment variableGITHUB_OWNERenvironment variableownerin the provider configurationThat is,
organizationoverridesGITHUB_ORGANIZATION(as I'd expect), butGITHUB_OWNERoverridesowner(that seems backwards).Changing this behaviour seems desirable, but as it would be a backwards incompatible change, the Hashicorp versioning guidelines suggest we should only do so as part of a major release. Therefore, I have not changed the configuration precedence in this pull request, but I have put a warning in the documentation saying we might do so in future.