-
Notifications
You must be signed in to change notification settings - Fork 172
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
fix(ui): correctly display AnalysisRun YAML #1861
Conversation
Signed-off-by: Hidde Beydals <hidde@hhh.computer>
✅ Deploy Preview for docs-kargo-akuity-io ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -32,7 +32,7 @@ export const AnalysisRunModal = ({ visible, hide, name }: Props) => { | |||
{isLoading ? ( | |||
<LoadingState /> | |||
) : ( | |||
<YamlEditor value={yaml.stringify(data?.analysisRun?.toJson())} height='500px' disabled /> | |||
<YamlEditor value={yaml.stringify(data?.result?.value?.toJson())} height='500px' disabled /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use new query parameter for using raw yaml
manifest in this PR? (Not sure if frontend maintainers like this code or not 👀)
const { data, isLoading } = useQuery(getAnalysisRun, {
namespace: projectName,
name,
format: RawFormat.YAML,
});
let manifest: string = '';
if (data?.result) {
manifest = new TextDecoder().decode(data.result.value as Uint8Array);
}
...
<YamlEditor value={manifest} height='500px' disabled />
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@devholic I think that would be more straightforward as well, but since this is broken right now, can we possibly create a follow-up for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, no problem :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following up was my thinking as well. Will do this later today. Thanks both 🙇🏼♂️
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1861 +/- ##
=======================================
Coverage 46.15% 46.15%
=======================================
Files 213 213
Lines 13865 13865
=======================================
Hits 6400 6400
Misses 7181 7181
Partials 284 284 ☔ View full report in Codecov by Sentry. |
@@ -32,7 +32,7 @@ export const AnalysisRunModal = ({ visible, hide, name }: Props) => { | |||
{isLoading ? ( | |||
<LoadingState /> | |||
) : ( | |||
<YamlEditor value={yaml.stringify(data?.analysisRun?.toJson())} height='500px' disabled /> | |||
<YamlEditor value={yaml.stringify(data?.result?.value?.toJson())} height='500px' disabled /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@devholic I think that would be more straightforward as well, but since this is broken right now, can we possibly create a follow-up for that?
Follow up to #1827.
Ensures the modal actually displays the YAML, even if it does not make use of the raw response API option yet.