search

cagov / data-infrastructure

CalData infrastructure
https://cagov.github.io/data-infrastructure
MIT License
7 stars 0 forks source link

upgraded packages.yml file #405

Closed britt-allen closed 1 month ago

britt-allen commented 1 month ago

Can we discuss this new terraform validation check? It seems to be a recent add, but I don't recall us discussing as a team. @ian-r-rose

ian-r-rose commented 1 month ago

We discussed it in #319, which moved the terraform check from a pre-commit check to a regular CI check. @JamesSLogan can you take a look at this failure? I believe the issue is that CI does not have commit permissions on branches. I think it's okay to not have that permission set, so maybe it's enough to just run git diff --exit-code instead of trying to commit.

britt-allen commented 1 month ago

If i understand correctly, you're saying to run git diff --exit code instead of committing files? I ask because I am creating a new branch for a separate PR soon. @ian-r-rose

ian-r-rose commented 1 month ago

Yes, --exit-code means that it will fail if there are differences (I originally had a typo)

JamesSLogan commented 1 month ago

Interesting...I'm not sure what is different about this PR/branch that would cause the job to fail. It was able to commit files here, for one example. Committing is part of the terraform-docs automation, so making the job simply fail if there are differences means the developer will have to have terraform installed locally to resolve the issue, which undoes the original intention of #319.

The code is ugly, doesn't add much value, and seems to be causing problems now. Maybe we drop the terraform-docs functionality after all?

ian-r-rose commented 1 month ago

Interesting...I'm not sure what is different about this PR/branch that would cause the job to fail. It was able to commit files here, for one example. Committing is part of the terraform-docs automation, so making the job simply fail if there are differences means the developer will have to have terraform installed locally to resolve the issue, which undoes the original intention of #319.

To be honest, I'm not sure why it succeeded there! I was under the impression that for GitHub actions to have commit permissions, you had to explicitly mark it as such.

The code is ugly, doesn't add much value, and seems to be causing problems now. Maybe we drop the terraform-docs functionality after all?

I could go either way on this. @britt-allen if you wanted to remove the terraform docs here, all you would have to do is delete these lines: https://github.com/cagov/data-infrastructure/blob/18c68843f7fbc9d7c008ac10a6fea8772f6277ba/.github/workflows/terraform-validation.yml#L33-L62

britt-allen commented 1 month ago

Interesting...I'm not sure what is different about this PR/branch that would cause the job to fail. It was able to commit files here, for one example. Committing is part of the terraform-docs automation, so making the job simply fail if there are differences means the developer will have to have terraform installed locally to resolve the issue, which undoes the original intention of #319.

To be honest, I'm not sure why it succeeded there! I was under the impression that for GitHub actions to have commit permissions, you had to explicitly mark it as such.

The code is ugly, doesn't add much value, and seems to be causing problems now. Maybe we drop the terraform-docs functionality after all?

I could go either way on this. @britt-allen if you wanted to remove the terraform docs here, all you would have to do is delete these lines:

https://github.com/cagov/data-infrastructure/blob/18c68843f7fbc9d7c008ac10a6fea8772f6277ba/.github/workflows/terraform-validation.yml#L33-L62

I somehow missed this comment, but still figured it out. Thank you Ian!