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.

Sign up for GitHub

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

Jump to bottom

feat: make terra plan write diff to github PR #383

Merged
merged 5 commits into from
Nov 12, 2024
Merged

feat: make terra plan write diff to github PR #383

merged 5 commits into from
Nov 12, 2024

Conversation

oneingan
Copy link
Contributor

@oneingan oneingan commented Jul 26, 2024 •
edited
Loading

Introduce postDiffToGitHubSnippet for unified diff posting and update kubectl.nix and terra.nix

  • Add postDiffToGitHubSnippet:

    • Created a reusable snippet that handles posting unified diffs to GitHub PR comments using the gh CLI.
    • Manages existing comments by updating them or creates new ones if necessary.

  • Update kubectl.nix:

    • Replaced inline diff posting logic with the new postDiffToGitHubSnippet.

  • Modify terra.nix:

    • Integrated postDiffToGitHubSnippet into the plan command within the wrap function.
    • Exported TF_VAR_fragment and TF_VAR_fragmentRelPath for use in Terraform configurations.
    • Changed the Terraform working directory to $PRJ_ROOT/.cache/${fragmentRelPath}/.tf for better organization.

@blaggacao
Copy link
Collaborator

Could you take it over and drive it over the finish line, especially with some real testing towards the end?

@oneingan oneingan force-pushed the feat/terra-diff-to-pr branch from aa5337f to d14933d Compare July 27, 2024 17:35
@oneingan
Copy link
Contributor Author

Currently works!

But i'm still not comfortable with --edit-last option from a monorepo point of view. Different terra plans overwrite between them. I will back with an alternative.

@oneingan oneingan force-pushed the feat/terra-diff-to-pr branch 6 times, most recently from cf1f888 to 0f52ee9 Compare July 30, 2024 16:38
@oneingan oneingan force-pushed the feat/terra-diff-to-pr branch from 0f52ee9 to 3bbd004 Compare October 2, 2024 22:19
@blaggacao
Copy link
Collaborator

@oneingan Nice! I'm noticing some movement. Go for it! 🤝

@oneingan oneingan force-pushed the feat/terra-diff-to-pr branch from 2f4df74 to 406bfe2 Compare October 18, 2024 01:44
@oneingan
Copy link
Contributor Author

oneingan commented Oct 18, 2024 •
edited
Loading

i make it append multiple plans in a sticky PR comment, also passthru some cell info in TF_VAR. I move the staging area to a different directory than nix and use full fragment path to avoid conflicts.

I thought is ready for review @blaggacao

@oneingan
Copy link
Contributor Author

image

@oneingan oneingan force-pushed the feat/terra-diff-to-pr branch 3 times, most recently from 52ef13e to cbbd463 Compare October 24, 2024 23:11
@blaggacao
Copy link
Collaborator

blaggacao commented Oct 25, 2024 •
edited
Loading

I (finally) made sure tests run on main: https://github.com/divnix/std/actions/runs/11517292880 — can you rebase?

@oneingan
Introduce postDiffToGitHubSnippet for unified diff posting and upda…
ad81098 
…te `kubectl.nix` and `terra.nix`

- **Add `postDiffToGitHubSnippet`:**
  - Created a reusable snippet that handles posting unified diffs to GitHub PR comments using the `gh` CLI.
  - Manages existing comments by updating them or creates new ones if necessary.

- **Update `kubectl.nix`:**
  - Replaced inline diff posting logic with the new `postDiffToGitHubSnippet`.

- **Modify `terra.nix`:**
  - Integrated `postDiffToGitHubSnippet` into the `plan` command within the `wrap` function.
  - Exported `TF_VAR_fragment` and `TF_VAR_fragmentRelPath` for use in Terraform configurations.
  - Changed the Terraform working directory to `$PRJ_ROOT/.cache/${fragmentRelPath}/.tf` for better organization.
@oneingan
add summary
f2e1fda
@oneingan
use new diffSnippet function
d2b2f53
@oneingan oneingan force-pushed the feat/terra-diff-to-pr branch from cbbd463 to d2b2f53 Compare October 25, 2024 17:49
@blaggacao
Copy link
Collaborator

blaggacao commented Nov 9, 2024 •
edited
Loading

Can you give some context on the CI failure?

It might be an accidental, unrelated failure. But I lack a bit of context on that possibility.

If that's the case, I'd have to apologize and fix that first.

@oneingan oneingan force-pushed the feat/terra-diff-to-pr branch from b90d8f7 to ad7f6ac Compare November 11, 2024 10:56
@oneingan oneingan force-pushed the feat/terra-diff-to-pr branch from ad7f6ac to e799c3f Compare November 11, 2024 11:06
@blaggacao
Copy link
Collaborator

blaggacao commented Nov 11, 2024 •
edited
Loading

Let's ignore the Mac failure, seems pretty much unrelated. I'm glad that linux is green now, thank you!

Please tackle any further fixes in follow up PRs, in case it's necessary. Merging. 🚀

@oneingan oneingan force-pushed the feat/terra-diff-to-pr branch from 69271c6 to 95f198a Compare November 11, 2024 20:17
@blaggacao
Copy link
Collaborator

Hm, the nvfetcher change, adding '''' looks quite unrelated, could you drop that?

@oneingan oneingan force-pushed the feat/terra-diff-to-pr branch from 95f198a to 216d7ca Compare November 11, 2024 20:47
@oneingan
Copy link
Contributor Author

sorry i was trying to catch the macos fail, i will revert last commits then

blaggacao
blaggacao approved these changes Nov 12, 2024
View reviewed changes
Copy link
Collaborator

@blaggacao blaggacao left a comment

Choose a reason for hiding this comment

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

LGTM

@blaggacao blaggacao merged commit e2b20a9 into divnix:main Nov 12, 2024
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blaggacao blaggacao blaggacao approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@oneingan @blaggacao