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

PXP-670: [CLI] Implement the AppSignal integration to the deployment status command #198

Merged
merged 15 commits into from
Mar 8, 2024

Conversation

jk-gan
Copy link
Contributor

@jk-gan jk-gan commented Feb 28, 2024

Summary

Screenshot 2024-02-28 at 3 44 46 PM

The PR also include the error handling when the application config is not enabled or when the appsignal integration is not enabled.
Screenshot 2024-02-28 at 3 45 12 PM
Screenshot 2024-02-28 at 3 44 59 PM

Ticket: PXP-670

What's Changed

Added

  • add wukong deployment status command

Fixed

  • fix the error handling for application config file
  • all the test cases are using the application config file

Changed

  • most of the commands now only work in the directory that contains .wukong.toml file

@jk-gan jk-gan requested review from mfauzaan, Fadhil and onimsha and removed request for mfauzaan February 28, 2024 07:47
@jk-gan jk-gan self-assigned this Feb 28, 2024
@jk-gan jk-gan added the feature New feature label Feb 28, 2024
@onimsha
Copy link
Contributor

onimsha commented Mar 4, 2024

@jk-gan I got this error while trying to test the feature

image

I understand this PR does not cover the init logic, but I wonder how did you test it ?

@onimsha
Copy link
Contributor

onimsha commented Mar 4, 2024

@jk-gan I got this error while trying to test the feature

image

I understand this PR does not cover the init logic, but I wonder how did you test it ?

I know how to get past this error. Now I got this

image

Copy link
Contributor

@onimsha onimsha left a comment

Choose a reason for hiding this comment

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

There are two major takeaways from this PR:

  • The CLI is still expecting the config application_name in the global config. We will need to modify it to use the application config exclusively.
  • The CLI is possibly giving wrong data from AppSignal. The output is not matching with the mockup and the magic link is pointing to some Unknown deployment in AppSignal.

We will need to fix these issues before merging this PR.

@jk-gan
Copy link
Contributor Author

jk-gan commented Mar 4, 2024

AppSignal doesn't recognize the latest deploy marker from the url param, so I will use incident_marker=last for the link

@onimsha
Copy link
Contributor

onimsha commented Mar 5, 2024

@jk-gan can you match the actual UI with the mockup as closet possible ? At least I want to see the APM as table title.

For the Prod, I think we can just show the name of the AppSignal Environment ? It will be clearer to developer about where we're pulling data from.

image

@jk-gan jk-gan requested a review from onimsha March 5, 2024 10:40
@sauron-droid
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jk-gan, onimsha

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@onimsha onimsha merged commit 05ac637 into main Mar 8, 2024
5 checks passed
@sauron-droid sauron-droid deleted the feat/deployment-status branch March 8, 2024 03:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants