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

show embedded template diffs line-by-line #23851

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

amirabbas8
Copy link
Contributor

Hi team,

I’ve developed a proof of concept for displaying template differences in the job plan output. Instead of modifying the API fields, I handled the changes directly in the client. Specifically, I added a conditional check in the final step where the field is edited. This check determines whether the field name is EmbeddedTmpl and, if so, utilizes a library to display the diff.

I’d appreciate your feedback on this approach. Do you agree with handling the diff in both the command-line interface and the web UI, or would you prefer modifying the API instead? Additionally, are there any other suggestions or improvements you would recommend?

Thank you for your time.

Screenshot CMD Screenshot Web

#23603

Copy link

hashicorp-cla-app bot commented Aug 22, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

CLA assistant check

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes


Amir Abbas seems not to be a GitHub user.
You need a GitHub account to be able to sign the CLA.
If you have already a GitHub account, please add the email address used for this commit to your account.

Have you signed the CLA already but the status is still pending? Recheck it.

@amirabbas8
Copy link
Contributor Author

Hey @tgross,
Just checking if there will be a review of this merge request. 🙏🏻
Thanks!

@tgross tgross self-requested a review September 23, 2024 15:45
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

Hi @amirabbas8!

The approach here looks great. We could do this on the server-side, but then the API client would need to transform the multi-line output anyways, so I think you've made the right choice here.

I'm a little more fuzzy on the UI side, so I'm going to tag in my colleague @philrenaud for his thoughts on the code and the new dependency you've introduced.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

2 participants