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

Add support for default merge commit params #1263

Conversation

garrettheel
Copy link
Contributor

@garrettheel garrettheel commented Aug 24, 2022

Alternative to #1201, adds support for:

  • squash_merge_commit_title
  • squash_merge_commit_message
  • merge_commit_title
  • merge_commit_message

Note: there are currently some build errors with github.DependabotEncryptedSecret that depend on #1258. Will rebase after that is merged.

@gesellix
Copy link
Contributor

@garrettheel #1258 has been merged.

@kfcampbell
Copy link
Member

Once this PR is rebased, I can merge/release!

@garrettheel garrettheel force-pushed the add-support-default-merge-commit-values branch from dae2b5f to c2f019a Compare August 30, 2022 19:24
@garrettheel
Copy link
Contributor Author

@kfcampbell I just rebased and bumped go-github up to v47

Copy link
Member

@kfcampbell kfcampbell left a comment

Choose a reason for hiding this comment

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

Integration tests for these items are failing on main and failing here in the exact same way, so it's not a regression!

Thanks for doing this.

@kfcampbell kfcampbell merged commit cbb3d7e into integrations:main Aug 30, 2022
@garrettheel garrettheel deleted the add-support-default-merge-commit-values branch August 31, 2022 03:34
@scruplelesswizard
Copy link

scruplelesswizard commented Sep 1, 2022

@kfcampbell @garrettheel It looks like this change may not have considered repos that have merge and squash disabled (i.e. rebase only). It appears there is no way to not set these values, as "" is not a valid value, and nulling them applies the defaults.

I'm happy to throw up a PR, but first wanted to ask, is there a preferred way to patch this?

@kfcampbell
Copy link
Member

@chaosaffe Hmm...I'm not sure! I don't know off the top of my head, sorry.

@garrettheel
Copy link
Contributor Author

garrettheel commented Sep 7, 2022

@chaosaffe would you mind creating a new issue to discuss? I'm curious what the negative impact there is in setting those defaults (is it just that a plan is generated?). I also don't know the answer off the top of my head, but happy to look into it with you

@danielpops
Copy link
Contributor

Perhaps an answer to the above question(s), with the shipping of this PR, my terraform plans suddenly show this diff for every repo in my organization:

  # github_repository.repo["the_repo"] will be updated in-place
  ~ resource "github_repository" "repo" {
        id                          = "the_repo"
      + merge_commit_message        = "PR_TITLE"
      + merge_commit_title          = "MERGE_MESSAGE"
        name                        = "the_repo"
      + squash_merge_commit_message = "COMMIT_MESSAGES"
      + squash_merge_commit_title   = "COMMIT_OR_PR_TITLE"
        # (28 unchanged attributes hidden)
}

This does not seem useful or expected for me, and I imagine it may confuse others.

@garrettheel
Copy link
Contributor Author

@danielpops @chaosaffe I created #1295 to track your concerns. Please use that to discuss.

kazaker pushed a commit to auto1-oss/terraform-provider-github that referenced this pull request Dec 28, 2022
Co-authored-by: Keegan Campbell <me@kfcampbell.com>
avidspartan1 pushed a commit to avidspartan1/terraform-provider-github that referenced this pull request Feb 5, 2024
Co-authored-by: Keegan Campbell <me@kfcampbell.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants