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

HCD: Implement nomad job status display #16

Merged
merged 3 commits into from
Oct 6, 2021
Merged

HCD: Implement nomad job status display #16

merged 3 commits into from
Oct 6, 2021

Conversation

jawahars16
Copy link
Contributor

What is this about?

This PR adds the nomad job status view to Damon, which can be displayed when hitting i on a selected job in the jobs view. The status gives the exact same response as the command nomad job status <job-id>

Preview

nomad-job-status

@hcjulz
Copy link
Collaborator

hcjulz commented Sep 29, 2021

Hey @jawahars16, that is amazing! Thx for this PR. I just glanced over it and it looked good.. I'll give it a proper review until the end of the day.

Copy link
Collaborator

@hcjulz hcjulz left a comment

Choose a reason for hiding this comment

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

Hey @jawahars16, thank you so much for your PR 👏 This is a huge help. The PR looks good in general but I added a handful of comments. Let me know if these make sense to you and if you have any questions just let me know.

cmd/damon/main.go Outdated Show resolved Hide resolved
view/view.go Outdated Show resolved Hide resolved
view/job_status.go Outdated Show resolved Hide resolved
component/job_status.go Outdated Show resolved Hide resolved
 - Used API instead of shell command to fetch Job Status
 - Added tablewriter to format the output
 - Added job status data into a separate model
@jawahars16
Copy link
Contributor Author

Hi @hcjulz I have fixed the feedbacks and updated the PR. Can you please take a look and let me know?

Copy link
Collaborator

@hcjulz hcjulz left a comment

Choose a reason for hiding this comment

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

Hey @jawahars16, this looks really amazing now!
I added a few really minor comments that I think could be addressed and then we can ship 🚢 this! Looking really forward to merging it.

component/job_status.go Outdated Show resolved Hide resolved
component/job_status.go Outdated Show resolved Hide resolved
component/job_status.go Outdated Show resolved Hide resolved
component/job_status.go Outdated Show resolved Hide resolved
component/job_status.go Outdated Show resolved Hide resolved
component/job_status.go Outdated Show resolved Hide resolved
nomad/job_status.go Outdated Show resolved Hide resolved
nomad/job_status.go Show resolved Hide resolved
 - Reused header constants
 - Using existing GetJob function
 - Added job ID to title
@jawahars16
Copy link
Contributor Author

Updated the PR. @hcjulz 🤞🏽

@hcjulz hcjulz merged commit 542c79a into hashicorp:main Oct 6, 2021
@hcjulz
Copy link
Collaborator

hcjulz commented Oct 6, 2021

Great Job @jawahars16 👏 I just merged your PR 🚀

@jawahars16
Copy link
Contributor Author

Thanks @hcjulz 😄 Can you also please remove the label in review?

@hcjulz hcjulz added done and removed in review labels Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants