Skip to content

Provide a default service_name label for profilecli uploads #2645

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

Merged
merged 3 commits into from
Nov 7, 2023

Conversation

aleks-p
Copy link
Contributor

@aleks-p aleks-p commented Nov 6, 2023

See #2642 for more info.

@aleks-p aleks-p requested a review from a team as a code owner November 6, 2023 14:51
@CLAassistant
Copy link

CLAassistant commented Nov 6, 2023

CLA assistant check
All committers have signed the CLA.

cmd.Flag("extra-labels", "Add additional labels to the profile(s)").Default("job=profilecli-upload").StringMapVar(&params.extraLabels)
cmd.Flag("extra-labels", "Add additional labels to the profile(s)").StringMapVar(&params.extraLabels)
Copy link
Contributor Author

@aleks-p aleks-p Nov 6, 2023

Choose a reason for hiding this comment

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

the job label is not used in this context based on my discussion with @cyriltovena, but let me know if you have any concerns with this change

@Rperry2174
Copy link
Contributor

awesome! I merged #2631 (comment) if you can rebase and also update docs as part of this pr would be appreciated!

Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

@aleks-p aleks-p force-pushed the profilecli-default-service-name branch from 61a89ba to 30be121 Compare November 6, 2023 15:49
@aleks-p aleks-p requested a review from a team as a code owner November 6, 2023 15:49
@Rperry2174
Copy link
Contributor

Great start! I have a few suggestions that could help improve the clarity and usability of the docs for users across different platforms.

Installation Instructions:

Could you clarify the installation steps for profile-cli? It would be beneficial to include explicit instructions on how users can install this utility on their system.

  • For macOS users, is it brew install pyroscope-io/brew/pyroscope or brew install pyroscope-io/brew/profile-cli or both?
  • For Linux users or non-macOS CI pipelines, could you provide the equivalent installation commands?

Adding a section that walks through the installation process before diving into the usage would be helpful.

Working Example:

A working example is always helpful as well. Could you add a complete, executable example to the examples/api folder on GitHub? This would give users a quick way to see profile-cli in action.

Separate Installation Documentation:

The installation instructions might be better suited for a separate page. While this could be a subsequent improvement, let's ensure that the current document is as complete and stand-alone as possible. We can later refactor the content into separate, more detailed page

The goal is to make the documentation intuitive enough so that when users ask "How do I install/use profile-cli?", they can be directed to both the docs and a comprehensive guide that works for their specific setup without additional explanations

@aleks-p
Copy link
Contributor Author

aleks-p commented Nov 6, 2023

Good idea @Rperry2174, I made #2650 to separate the work done here and extending the docs further.

@aleks-p aleks-p merged commit 3ef2625 into main Nov 7, 2023
@aleks-p aleks-p deleted the profilecli-default-service-name branch November 7, 2023 13:00
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.

4 participants