search

hashicorp / terraform-aws-consul

A Terraform Module for how to run Consul on AWS using Terraform and Packer
Apache License 2.0
401 stars 484 forks source link

Fix AMI names #194

Closed brikis98 closed 3 years ago

brikis98 commented 3 years ago

This is yet another fix I originally pushed to https://github.com/hashicorp/terraform-aws-consul/pull/193... But for some reason, when I do an internal release on a branch, the PR on that branch gets auto merged? I can't figure out what's doing that...

Update: I've also removed the logic to write the published AMI IDs to a Markdown file in this PR, as it provides too little value, while taking a lot of work to maintain.

yorinasub17 commented 3 years ago

Two thoughts:

brikis98 commented 3 years ago

Hm, I guess so, but it's not obvious to me why updating a couple .md files in master results in a PR being merged? Even more bewilderingly, this git-commit-add-push step is exactly what failed on the most recent build, so how the hell did it work before?

  • You should mark the release as pre-release so it doesn't show up as latest (just did it for the last one you did).

I did just that!

Screen Shot 2020-10-16 at 10 40 36 AM
brikis98 commented 3 years ago

Alright, deploy step is finally passing! This is ready for review.

yorinasub17 commented 3 years ago

Hm, I guess so, but it's not obvious to me why updating a couple .md files in master results in a PR being merged?

Oh hmm... I can't find the documentation for this, but I think this has to do with the fact that on tag based builds in CircleCI, master branch actually contains all the code in the tag. You can verify this by starting an SSH build, SSH in, and checking out master and doing git log.

brikis98 commented 3 years ago

Hm, I guess so, but it's not obvious to me why updating a couple .md files in master results in a PR being merged?

Oh hmm... I can't find the documentation for this, but I think this has to do with the fact that on tag based builds in CircleCI, master branch actually contains all the code in the tag. You can verify this by starting an SSH build, SSH in, and checking out master and doing git log.

Hm, even if so, why does committing code to master count the same as merging a PR?

At any rate, the commit code is removed in this PR, so I'm going to merge it.

yorinasub17 commented 3 years ago

Hm, even if so, why does committing code to master count the same as merging a PR?

Oh that one I can explain. Assuming that circleci build checkout master branch contains commits from the tag:

brikis98 commented 3 years ago
  • Because master now includes the exact commits from the branch, github treats that as a merge event for the branch, and closes the PR as merged (because the commits from the PR branch made it to master, the target).

This is the part that I wasn't aware of. I always assumed that a PR was a standalone entity and that only explicitly doing a merge action in that PR would it be closed. The fact that GitHub detects that the same commits were already merged and closes the PR as merged is news to me. Does that mean that if I open 10 PRs with exactly the same code and merge one in, they all close as merged? Anyway, this is just a curiosity, thx for the explanation.

yorinasub17 commented 3 years ago

Does that mean that if I open 10 PRs with exactly the same code and merge one in, they all close as merged?

Yup. This also happens when we run testpr on a fork (which creates pull-request-123 in the repo), extend the branch with additional commits to open a new PR, and merge that in, it will close the original PR as merged (giving the contributor credit).

brikis98 commented 3 years ago

Roger!