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.

Sign up for GitHub

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

Jump to bottom

feat: add support for tracing package types #5252

Merged
merged 14 commits into from Jan 26, 2024
Merged

feat: add support for tracing package types #5252

merged 14 commits into from Jan 26, 2024

Conversation

jbw976
Copy link
Member

@jbw976 jbw976 commented Jan 17, 2024 •
edited by phisco

Description of your changes

This PR adds support to the crossplane beta trace command to trace and display details of package types, such as Provider, Configuration, and Function.

Some details about the experience so far can be found in #5028 (comment).

This PR will remain in draft while there are more tasks to complete, but we wanted to get early eyes on it as we're approaching the v1.15 milestone.

TODO:

  • versions of packages/revisions aren't being shown anywhere
  • -o dot is not implemented yet, just default, wide, json
  • no unit tests have been written at all
  • refactor some inefficient code and long/ugly getPackageTree function

Fixes #5028

I have:

Need help with this checklist? See the cheat sheet.

@phisco phisco marked this pull request as ready for review January 17, 2024 17:40
@phisco phisco requested review from phisco and a team as code owners January 17, 2024 17:40
@phisco phisco requested a review from turkenh January 17, 2024 17:40
@phisco
Copy link
Contributor

phisco commented Jan 17, 2024

FYI: I didn't want to mark this as ready for review obviously... but I can't find how to undo that from phone.

@jbw976 jbw976 marked this pull request as draft January 17, 2024 19:10
lsviben
lsviben reviewed Jan 19, 2024
View reviewed changes
Copy link
Contributor

@lsviben lsviben left a comment

Choose a reason for hiding this comment

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

Did a quick glance.

Thanks for the comment in #5028 about how it works. I think the behaviour is as expected.

Left some minor comments, as I didnt touch the meat in getPackageTree which you have planned to refactor, so not sure if there is sense to check it before (let us know). Plus some things are already noted by lint.

BTW I see you have also a task to add -o dot support. Not sure how time-consuming that is but you could keep it as a follow-up PR maybe?

cmd/crank/beta/trace/internal/printer/default.go Show resolved Hide resolved
cmd/crank/beta/trace/trace.go Outdated Show resolved Hide resolved
@jbw976
feat: add support for tracing package types
5e291cf 
Signed-off-by: Jared Watts <jbw976@gmail.com>
@lsviben
lint fixing
e7c6791 
Signed-off-by: lsviben <sviben.lovro@gmail.com>
@lsviben
reduce complexity of getPackageTree
a6d360d 
Signed-off-by: lsviben <sviben.lovro@gmail.com>
@lsviben
Copy link
Contributor

lsviben commented Jan 23, 2024

I pushed some changes, mostly around fixing the lint errors and simplifying getPackageTree. Will work on this further, but I dont think it will be ready in time for the 1.15 feature freeze

lsviben and others added 6 commits January 25, 2024 13:03
@lsviben
improve flags to be clear they are for packages
5ac32f0 
Signed-off-by: lsviben <sviben.lovro@gmail.com>
@lsviben
extract getting package deps
1fb0221 
Signed-off-by: lsviben <sviben.lovro@gmail.com>
@lsviben
last improvements on getPackageTree
6208059 
Signed-off-by: lsviben <sviben.lovro@gmail.com>
@lsviben
start on unit tests
902fe57 
Signed-off-by: lsviben <sviben.lovro@gmail.com>
@phisco
feat: show packages image in wide, version always
b504b9d 
Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
@phisco
tests: add package printing some coverage
195f57e 
Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
@phisco
Copy link
Contributor

phisco commented Jan 25, 2024

A few screenshots of the current output.

Here without images, only showing the version of each package/revision:
image

With -o wide, it shows also the image (and full messages in the status if any):
image

Here with actual issues:
image

@phisco phisco marked this pull request as ready for review January 25, 2024 18:37
@phisco
feat: support dot output
ba6fa55 
Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
@phisco
refactor: move clients to separate packages
51f1cde 
Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
@phisco
refactor: simplify xpkg tree gathering logic
bd76202 
Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
@phisco
feat: support --show-packager-runtime-configs
72dd13b 
Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
@phisco
tests: add client coverage
d75ab6c 
Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
@phisco phisco mentioned this pull request Jan 26, 2024
Closed
phisco
phisco approved these changes Jan 26, 2024
View reviewed changes
Copy link
Contributor

@phisco phisco left a comment

Choose a reason for hiding this comment

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

I'm obviously biased, but I think it's now ok to merge :shipit: We should squash the commits before merging though.

lsviben
lsviben approved these changes Jan 26, 2024
View reviewed changes
Copy link
Contributor

@lsviben lsviben 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 @jbw976 and @phisco . I am also a bit biased, but IMO this feature will be really helpful!

LGTM

@phisco phisco merged commit 2078555 into crossplane:master Jan 26, 2024
18 of 19 checks passed
negz pushed a commit to negz/crossplane that referenced this pull request Jan 31, 2024
@jbw976 @lsviben
@phisco @negz
feat(crank/trace): add support for package types (crossplane#5252)
6764c66 
Signed-off-by: Jared Watts <jbw976@gmail.com>
Signed-off-by: lsviben <sviben.lovro@gmail.com>
Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
Co-authored-by: Jared Watts <jbw976@gmail.com>
Co-authored-by: lsviben <sviben.lovro@gmail.com>
Co-authored-by: Philippe Scorsolini <p.scorsolini@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phisco phisco phisco approved these changes

@lsviben lsviben lsviben approved these changes

@turkenh turkenh Awaiting requested review from turkenh turkenh is a code owner automatically assigned from crossplane/crossplane-maintainers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

trace should understand the package resource model to help troubleshoot them also
3 participants
@jbw976 @phisco @lsviben