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

fix: Console Pod Logs broken if not in a plural application #348

Merged
merged 2 commits into from
Apr 29, 2023

Conversation

dogmar
Copy link
Collaborator

@dogmar dogmar commented Apr 27, 2023

Summary

Created a new 'Logs' tab for Pods pages that have no associated apps. Pods without an associated apps will have the 'View logs' button direct to this tab while pods with apps will still be linked to the App page's logs tab.

Labels

Test Plan

Checklist

  • If required, I have updated the Plural documentation accordingly.
  • I have added tests to cover my changes.
  • I have added a meaningful title and summary to convey the impact of this PR to a user.
  • I have added relevant labels to this PR to help with categorization for release notes.

@dogmar dogmar added bug-fix This pull request fixes a bug frontend Changes related to the frontend labels Apr 27, 2023
@github-actions
Copy link
Contributor

Visit the preview URL for this PR (updated for commit cb75b75):

https://pluralsh-console--pr348-klink-pod-logs-fix-rzyrdrfw.web.app

(expires Thu, 04 May 2023 20:38:56 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: dd1ffa0705acc6ef7d6db370e6bd6fc390e945ce

@dogmar dogmar marked this pull request as ready for review April 27, 2023 20:42
@dogmar dogmar requested review from maciaszczykm and a team April 27, 2023 20:42
name?: string
namespace?: string
}) {
const isApp = useNamespaceIsApp(namespace)
Copy link
Member

Choose a reason for hiding this comment

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

could we just pass this as an argument and ignore the potentially risky app name fetch?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is there some field on a Pod that I could use to determine if it's an app or not? Otherwise I'd just be pushing the app name fetch further up the render tree.

Copy link
Member

Choose a reason for hiding this comment

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

Not easily, I thought you might be able to determine it from context of where the component is called, eg if it's called from an /apps/ prefixed url use something like that link, otherwise non-app link?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nah, I need to know whether it's an app or not on the Pod page, where it's not obvious.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Either that, or we always show the logs on the Pod page, even if there is an associated App page where you could also view the logs.

Copy link
Member

@michaeljguarino michaeljguarino left a comment

Choose a reason for hiding this comment

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

Let's just try this for now, it should be accurate enough

@michaeljguarino michaeljguarino merged commit 55d33a3 into master Apr 29, 2023
@michaeljguarino michaeljguarino deleted the klink/pod-logs-fix branch April 29, 2023 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix This pull request fixes a bug frontend Changes related to the frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants