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

3.1.0 has breaking changes which are not identified in changelog #566

Closed
xorima opened this issue Oct 13, 2020 · 22 comments · Fixed by #592
Closed

3.1.0 has breaking changes which are not identified in changelog #566

xorima opened this issue Oct 13, 2020 · 22 comments · Fixed by #592
Milestone

Comments

@xorima
Copy link
Contributor

xorima commented Oct 13, 2020

Affected Resource(s)

  • github_branch_protection

Expected Behavior

Minor bumps should not have breaking changes, https://semver.org/

Actual Behavior

You have introduced undocumented breaking changes inside the provider without putting anything in the changelog:

  • branch has become pattern
  • repository seems to have become repository_id

Also there is no update to the documentation for this change

Please either revert this change or release a 4.0.0 with actual updated documentation.

References

@mbarany
Copy link
Contributor

mbarany commented Oct 13, 2020

I noticed the same. I had to pin the provider to 3.0

@mwarkentin
Copy link
Contributor

mwarkentin commented Oct 14, 2020

repository_id = github_repository.foo.node_id was the key for me doing the upgrade. I was able to update branch -> pattern without any changes.

I don't think node_id is documented that I could find, but it appears to be related to the upgrade to the v4 (graphql) api.

@anthonyangel
Copy link

We use a for_each loop to create our repositories, and a separate for_each loop to set up branch protection for 1 or more branches per repository. Being able to use the id (which was just the name) was nice & simple, but having to retrieve the node_id from the repository resource and apply that into the branch_protection resource looks like it's a pain.

@kchristensen
Copy link

Seems as though the restrictions argument to github_branch_protection was also renamed, somewhere between 2.9.0 and 3.1.0. We were still on 2.9.0 (mostly due to the lack of actual Github releases so I was unaware new versions were being released).

However the documentation doesn't seem to have been updated to reflect any changes.

@jcudit
Copy link
Contributor

jcudit commented Oct 17, 2020

😞 apologies on this one.


Please either revert this change or release a 4.0.0 with actual updated documentation.

I am inclined to revert the change and cut a new v3.x release.

Can I get some 👍 👎 feedback on this approach?

@Krenair
Copy link

Krenair commented Oct 18, 2020 via email

@nikolay
Copy link
Contributor

nikolay commented Oct 19, 2020

@jcudit Not to add to your plate, but we're still waiting for the org-level secrets! The feature was released half a year ago and is still unsupported.

@jcudit
Copy link
Contributor

jcudit commented Oct 19, 2020

☝️ reverts the changes that removed repository and branch. The new fields are now computed so we can keep the same implementation behind the v3 configuration interface. Breaking changes are now queued for the v4 release, hopefully around end of year.

I think this is a good compromise, but let me know if I have overlooked anything. If this plan does not work, I am still open to cutting a new major release or a more aggressive revert that re-instates the old implementation.

👎 / 👍 what do you all think?

@onwsk8r
Copy link

onwsk8r commented Oct 21, 2020

When you guys release v4, would it be possible to do something like

schema.Schema{
    "repository": { ConflictsWith: []string{"repository_id"} }
    "repository_id": { ConflictsWith: []string{"repository"} }
}

Because sometimes I don't have a resource "github_repo" "foo" handy and don't really want the extra data lookup

@jcudit
Copy link
Contributor

jcudit commented Nov 1, 2020

https://github.com/terraform-providers/terraform-provider-github/pull/570 has also grown to re-instate the restrictions and dismissal_users / dismissal_teams configuration blocks that were modified to produce breaking changes. However, a regression was found that was introduced when switching to the GraphQL API.

Re-instating REST client management of dismissal and push restrictions is the next step here. I'll experiment with what a dual REST / GraphQL resource would look like as well as the performance implications of this approach.

If this does not play out well, a full revert of the github_branch_protection resource back to v2.9.2 would be our final option to get things back to a functioning state and back to the v3.0.0 configuration schema.

@jcudit
Copy link
Contributor

jcudit commented Nov 1, 2020

I've raised https://github.com/terraform-providers/terraform-provider-github/pull/582 to follow through on fully reverting the GraphQL implementation of the github_branch_protection resource.

Rolling forward with a change was getting too complex. I unfortunately cannot take on the maintenance burden of a change like this. I've hit my limit for unknowns and edge cases with the current implementation and would appreciate going back to a more stable state. Perhaps we can revisit this as a separate resource or something more incremental in the future. For now, I'm hoping we can ship a revert and broaden focus to other parts of this project. Once again, 😞 the release turned out this way.

Because this reverts the schema back to v3.0.0, I'm aiming to release this under v3.1.1 and document v3.1.0 as unusable in the CHANGELOG. Releasing v4.0.0 is an option, but I'm hoping to reserve the major version bump for another set of changes. Happy to bend to the community's preference on this if y'all feel strongly about how this gets versioned.

@kennydo
Copy link

kennydo commented Nov 3, 2020

Being able to declare branch protection rules against branch names with wildcards has been a long awaited feature for me and my team, and it requires the GraphQL API. I started using the github provider for my project when v3.1.0 came out because I wanted to use terraform, but couldn't until this was supported.

If at all possible, it would be nice if this feature was added back sooner than the end of the year.

@nikolay
Copy link
Contributor

nikolay commented Nov 3, 2020

@jcudit Sorry, but there's no longer an option to revert in my book - many have built against v3.1 already! You made a mistake - we all do, but 3 weeks have passed since then, and it's too late to revert.

@kchristensen
Copy link

I'd tend to agree, I think there is a need to fail forward here. Not only is wildcard use in branch protection rules super common and something many of us using this provider have been waiting ages for, but many of us are using 3.1.0 with some amount of success and getting "stuck" on 3.1.0 or having to deal with state rm'ing and reimporting affected resources on another version of the provider is kind of a nightmare.

@xorima
Copy link
Contributor Author

xorima commented Nov 4, 2020

I think we should just cut a 4.0.0 release with valid release notes/documentation.

End of the day version numbers are free.

@jcudit
Copy link
Contributor

jcudit commented Nov 4, 2020

Solid feedback everyone ❤️ . Aiming to cut v4.0.0 with valid release notes / docs this week.

I'm also inclined to add a github_branch_protection_v3 resource for anyone that wants to stay current with other changes but hang in with the previous REST implementation and feature set. Hoping this provides a stable option as we work through issues with the GraphQL implementation.

@fishfacemcgee
Copy link

I'm not sure I understand what making a 4.0.0 release with this change does. Could someone clarify what the upgrade path for folks who are currently stuck on 3.0.0 is going to look like? Is it going to be this github_branch_protection_v3 resource?

@jcudit
Copy link
Contributor

jcudit commented Nov 4, 2020

@fishfacemcgee yes, the github_branch_protection_v3 would provide an option to continue with the v3.0.0 version of github_branch_protection.

A possible migration path would likely be to rewrite configuration to use github_branch_protection_v3 instead of github_branch_protection and import any existing github_branch_protection resources into the new github_branch_protection_v3 resource. Will 🤔 more on it, but this is the only option coming to mind at the moment.

From what I've read, the GraphQL implementation that has taken over the github_branch_protection resource is unsuitable from a performance standpoint for some users, but also contains a long-awaited feature that other users value more than performance implications. Adding complexity was also explored to support both implementations, but did not pan out well. This is unfortunate, as there are bound to be people hit with extra work because of the mistakes made.

Open to other ideas of what a good compromise looks like, but still pushing to get the v4.0.0 release out due to the majority of viewpoints expressed in this thread.

@nikolay
Copy link
Contributor

nikolay commented Nov 4, 2020

@jcudit The upgrade to a major version is voluntary. Why force people who are now on v3.1 already to refactor again? The whole github_branch_protection_v3 seems of no use to me given the update branch protection is the focus of the major release. What's the point to upgrade just for the sake to be on the latest version?

@jcudit jcudit added this to the v4.0.0 milestone Nov 5, 2020
@jcudit
Copy link
Contributor

jcudit commented Nov 5, 2020

@nikolay the major version upgrade being voluntary is a good point.

If anyone feels they'd benefit from a new resource pinned to the old implementation of github_branch_protection, please raise an issue and we'll work towards it. Will otherwise be following guidance in this thread to release the changes grouped in this milestone as v4.0.0.

@fishfacemcgee
Copy link

fishfacemcgee commented Nov 5, 2020

@jcudit Just so I'm on the same page, the stance now is "If you want the 3.1.0 features, you must work around the breaking changes?" Can we at least get the breaking changes documented now, since it affects the currently released version out in the wild, if that's the case?

@jcudit
Copy link
Contributor

jcudit commented Nov 6, 2020

@fishfacemcgee confirmed, that is the stance. Take a look at the PR linked above for documentation updates. Any feedback would be appreciated 🙇 .

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