Skip to content

Conversation

fabianwolski
Copy link
Contributor

Please review if url construction logic could be better implemented. Is link manager used with best practise?

https://github.tools.sap/kubernetes/k8s-lifecycle-management/issues/2049

Copy link
Contributor

@andreaskienle andreaskienle left a comment

Choose a reason for hiding this comment

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

Great job, Fabian! It's awesome to see the new issue fields prefilled. 🎉

Just a couple of minor notes, especially for the GitHub URL, let's make sure to add it to ⁠frontend-config.json since it's distinct from the documentation URL (as noted below).

@andreaskienle andreaskienle self-assigned this Apr 10, 2025
@fabianwolski
Copy link
Contributor Author

what would be the best approach for parameters for linkcreator constructor? I was going to make githubBaseUrl ?: (optional) But is there a better way? Image adding 10 different url's what then. (I pushed anyway with error just so you can take a look).

githubBaseUrl = https://github.tools.sap/openmcp (could be better name, also not sure?)

Thank you @andreaskienle @enrico-kaack-comp

@fabianwolski fabianwolski requested a review from andreaskienle May 2, 2025 16:25
andreaskienle
andreaskienle previously approved these changes May 2, 2025
Copy link
Contributor

@andreaskienle andreaskienle left a comment

Choose a reason for hiding this comment

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

Nice work! ✨

There are two unused text entries we can remove, and then it’s ready to be merged. 🎉

As a follow-up, it would be good to gracefully handle cases where the link configuration is missing — for example, by hiding the button when no link is available. But that’s something we can tackle in a separate task (there’s already a backlog item). For now, everything looks good!

Co-authored-by: Andreas Kienle <[email protected]>
Copy link
Contributor

@andreaskienle andreaskienle left a comment

Choose a reason for hiding this comment

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

👍🏻

@fabianwolski fabianwolski merged commit 2376211 into main May 2, 2025
4 checks passed
@fabianwolski fabianwolski deleted the feature/copy-status-info-pasting-into-support-channels branch May 2, 2025 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants