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

Add buildpack details to cf7 app summary #1971

Closed
wants to merge 2 commits into from

Conversation

piyalibanerjee
Copy link
Contributor

Co-authored-by: Piyali Banerjee pbanerjee@pivotal.io
Co-authored-by: Connor Braa cbraa@pivotal.io

Thank you for contributing to the CF CLI! Please read the following:

  • If you haven't yet, please review our contributing guidelines: https://github.com/cloudfoundry/cli/blob/master/.github/CONTRIBUTING.md
  • We're not allowed to accept any PRs without a signed CLA, no matter how small.
    If your contribution falls under a company CLA but your membership is not public, expect delays while we confirm.
  • All new code requires tests to protect against regressions.
  • Contributions must confirm to our style guide. Please reach out to us if you have questions.

Does this PR modify CLI v6 or v7?

v7

Please see the contribution doc above or review Architecture Guide.

Description of the Change and Value of PR

The app summary command sometimes lists buildpack names hardcoded in the buildpacks themselves (i.e.: "python" for any python buildpack). Since these names may be different from the user-provided names (for Custom Buildpacks), this may be confusing for users. This PR includes some additions to the buildpacks info in the app summary output to include the user-provided buildpack names as well as the buildpack versions if available. We kept the original buildpacks field the same in case other scripts rely on that already.

Slack discussion with CLI team: https://cloudfoundry.slack.com/archives/C032824SM/p1593561984360700

Why Should This Be In Core?

Explain why this functionality should be in the cf CLI, as opposed to a plugin.
Having detailed buildpack information in the app summary output is valuable in general for debugging purposes, as well as for ensuring the correct buildpacks were used to build the app.

Applicable Issues

List any applicable Github Issues here

How Urgent Is The Change?

Is the change urgent? If so, explain why it is time-sensitive.
No.

Other Relevant Parties

Who else is affected by the change?

Co-authored-by: Piyali Banerjee <pbanerjee@pivotal.io>
Co-authored-by: Connor Braa <cbraa@pivotal.io>
@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/173716617

The labels on this github issue will be updated when the story is started.

In the case of multi-buildpack, it seems only one buildpack populates
the `detect_output` field. However, after talking with the original
authors of the PR, it seems that `buildpack_name` in the default
provided buildpacks in this case. This gives us a better separation
between the `buildpack names:` and `buildpacks` fields.

[#173716617](https://www.pivotaltracker.com/story/show/173716617)

Co-authored-by: Sebastian Vidrio <svidrio@pivotal.io>
nickjameswebb added a commit that referenced this pull request Aug 18, 2020
- Code is mostly from this PR
#1971. PR was from before
branching strategy so it wasn't easy to merge into v7
- Info from CAPI buildpacks endpoint is put into a table, no more cli
opinion on what to show

[#173716617](https://www.pivotaltracker.com/story/show/173716617)

Co-authored-by: Steve Taylor <staylor@pivotal.io>
staylor14 added a commit that referenced this pull request Aug 19, 2020
- Code is mostly from this PR
#1971. PR was from before
branching strategy so it wasn't easy to merge into v7
- Info from CAPI buildpacks endpoint is put into a table, no more cli
opinion on what to show

[#173716617](https://www.pivotaltracker.com/story/show/173716617)

Co-authored-by: Steve Taylor <staylor@pivotal.io>
JenGoldstrich pushed a commit that referenced this pull request Aug 20, 2020
- Code is mostly from this PR
#1971. PR was from before
branching strategy so it wasn't easy to merge into v7
- Info from CAPI buildpacks endpoint is put into a table, no more cli
opinion on what to show

[#173716617](https://www.pivotaltracker.com/story/show/173716617)

Co-authored-by: Steve Taylor <staylor@pivotal.io>
@JenGoldstrich
Copy link
Contributor

I believe this functionality was covered in this commit 528d0ff as such I am closing this, please let me know if this has been closed in error,

Thanks,
Jenna

@moleske moleske deleted the enhanced-app-summary-buildpacks branch December 15, 2023 00:04
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