#pr-reviews (2021-02)

@Jeremy G (Cloud Posse) I can’t figure out why it doesn’t like it

we’ve recently merged other PRs that’ didn’t complain about code owners

That repo has old workflows. Somehow the mass update missed updating the workflows on that repo.

@Frank Please run make init; make github/init; make pr/auto-format and check in and push the changes.

@Jeremy G (Cloud Posse) Done!

@Erik Osterman (Cloud Posse) this module had never been converted to [context.tf](http://context.tf) and does not really need to be, since it does not call any other modules and takes an explicit ID (container_name). Furthermore, it already has an environment variable declared. I started to convert it, but then I thought maybe we should just leave it as not having [context.tf](http://context.tf) . What do you think?

Yea I do not think this module needs context tf

hey @Patrick Jahns thanks for following up

we’ve had some other PRs get in the way of this - mostly related to null label and addressing an issue that prevented backwards compatibility

@Jeremy G (Cloud Posse) please see @Patrick Jahns comment: https://github.com/cloudposse/terraform-aws-eks-node-group/pull/54#issuecomment-764994265

(we can discusson on monday)

@Erik Osterman (Cloud Posse) The question for you to resolve is how confusing are we going to allow this module to be, particularly since Terraform does not allow us to output custom error messages. You can only specify multiple instance types if you do not use some non-obvious set of other features which require a launch template. We have gone to great lengths to automatically create and configure a launch template if and only if needed to support a feature (for example, tagging created resources), and we do not have a great way of telling people that the reason their plan/apply is failing is that you cannot use both tagging and spot instances at the same time.

Good stuff Released as 0.54.0 (Should’ve been a patch, but I missed the label )

@Leia Renée reported this to me yesterday too

@Jeremy G (Cloud Posse) can you take a look point I think this is related to something you recently were looking at

@Erik Osterman (Cloud Posse) We need to use Github provider 3.0.0 because of breaking changes. We cannot use integrations/github because it does not have v3.0.0 (I think it starts at v4.2). However, the problem above seems due to the ~> 2.8.0 version pin of the GitHub provider. Not sure where that is coming from, but should be alleviated by updating all modules to the latest version.

Root modules should update their version pin to

    github = {
      source = "hashicorp/github"
      # breaking changes both immediately before and after 3.0.0, pin exactly
      # until ready to upgrade to integrations/github 4.2 or later
      version = "3.0.0"
    }

@Maxim Mironenko (Cloud Posse)

Basically, it wants to keep swapping the position of the S3 resource and the S3 data source link in the resources section.

Haven’t dug into the code yet, but wondering if anyone has seen this?

There’s a new version of terraform docs

I haven’t looked at this particular issue but chances are it hasn’t been updated somewhere

For posterity, we got around this temporarily by just reissuing the readme build via ChatOps, though I think that will also be non-deterministic with respect to the readme CI task

Hey @nnsense we’ve got some recently introduced hurdles to jump over in regards to GH actions that sometimes cause issues like in this PR where they don’t report status back. The only way I know to solve them to push another commit. See the comment here, follow those steps, and that should kick off that validate codeowners action which will enable us to get that PR merge — https://github.com/cloudposse/terraform-aws-rds/pull/106#issuecomment-784455208

Oops, sorry I’ve missed your answer on GH, will do, thanks

Done