Skip to content
This repository has been archived by the owner on Oct 10, 2023. It is now read-only.

Update the builder plugin to make version flag optional and add new 'build-plugin-local' make target #3289

Merged
merged 1 commit into from
Sep 21, 2022

Conversation

chandrareddyp
Copy link
Contributor

@chandrareddyp chandrareddyp commented Sep 8, 2022

What this PR does / why we need it

  1. Updates the command tanzu builder cli compile --match <package_name> --version logic to make the flag "--version" as optional and the help text of the flag.
  2. New make targets build-plugin-local and install-plugin-local has added to give flexibility to users to build and install the given plugin locally.

Which issue(s) this PR fixes

Fixes ##899

Describe testing done for PR

  1. The command tanzu builder cli compile --match package --version $(make version) --ldflags "-X 'github.com/vmware-tanzu/tanzu-framework/pkg/v1/buildinfo.Version=v0.27.0-dev'" tested with --ldflags having Version info, works fine.
  2. The command tanzu builder cli compile --match package ---ldflags "-X 'github.com/vmware-tanzu/tanzu-framework/pkg/v1/buildinfo.Version=v0.27.0-dev'" tested without --version flag, works fine.
  3. The new make target build-plugin-local tested: With valid plugin name make build-plugin-local PLUGIN_NAME=package works fine, builds the given plugin. Also tested with empty and in-valid PLUGIN_NAME, it does throw an error saying: The PLUGIN_NAME: '' is not valid or not exists or empty, please provide valid PLUGIN_NAME.
  4. The new make target install-plugin-local tested: With valid plugin name make install-plugin-local PLUGIN_NAME=login works fine, build and install the given plugin. Also tested with empty and in-valid PLUGIN_NAME, it does throw an error saying: The PLUGIN_NAME: '' is not valid or not exists or empty, please provide valid PLUGIN_NAME.

Release note

The command `tanzu builder cli compile` has been updated to make the flag `--version` as optional
New make target  `build-plugin-local` has been added to build the given plugin locally
New make target `install-plugin-local` has been added to build and install the given plugin locally

Additional information

Special notes for your reviewer

@chandrareddyp chandrareddyp requested a review from a team as a code owner September 8, 2022 22:30
@codecov
Copy link

codecov bot commented Sep 8, 2022

Codecov Report

Merging #3289 (1b5ad77) into main (8a526ce) will increase coverage by 6.06%.
The diff coverage is n/a.

❗ Current head 1b5ad77 differs from pull request most recent head 25d8570. Consider uploading reports for the commit 25d8570 to get more accurate results

@@            Coverage Diff             @@
##             main    #3289      +/-   ##
==========================================
+ Coverage   46.91%   52.98%   +6.06%     
==========================================
  Files         265      103     -162     
  Lines       28879    10419   -18460     
==========================================
- Hits        13550     5520    -8030     
+ Misses      14082     4440    -9642     
+ Partials     1247      459     -788     
Impacted Files Coverage Δ
pkg/v1/tkg/tkgconfigupdater/save.go
pkg/v1/tkg/cmd/root.go
pkg/v1/tkg/clusterclient/resource.go
pkg/v1/tkg/tkgctl/set_ceip.go
pkg/v1/tkg/cmd/config_cluster.go
pkg/v1/tkg/tkgctl/describe_cluster.go
pkg/v1/tkg/cmd/describe.go
pkg/v1/tkg/client/delete_region.go
pkg/v1/tkg/tkgconfigupdater/helper.go
pkg/v1/tkg/clusterclient/credentials.go
... and 153 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

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

This works for me. One comment in-line.

pkg/v1/builder/command/cli_compile.go Outdated Show resolved Hide resolved
@chandrareddyp chandrareddyp force-pushed the topic/cpamuluri/builder-cli-compile-fix branch 2 times, most recently from 1351c47 to 78b11c0 Compare September 13, 2022 02:02
@chandrareddyp chandrareddyp requested review from a team as code owners September 13, 2022 20:03
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3289/20220913201545/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@chandrareddyp chandrareddyp force-pushed the topic/cpamuluri/builder-cli-compile-fix branch from fc49623 to 4e256dd Compare September 13, 2022 20:44
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3289/20220913205639/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@chandrareddyp chandrareddyp changed the title Update the builder plugin logic to pass Version info to compile plugin Update the builder plugin to make version flag optional and add new 'build-plugin-local' make target Sep 13, 2022
@chandrareddyp chandrareddyp force-pushed the topic/cpamuluri/builder-cli-compile-fix branch from 4e256dd to d4b8064 Compare September 14, 2022 16:38
Copy link
Contributor

@anujc25 anujc25 left a comment

Choose a reason for hiding this comment

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

build-plugin-local will compile the plugin and put it in the artifacts directory within the repository.

We should add install-plugin-local that installs the plugin as well. Because as a end user I would like to do some change in the plugin, then I want to build as well as install the plugin to test my change.

Copy link
Contributor

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

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

Nit:

  • git recommends that the commit title (the first line) be 50 characters or less. This is useful because that line is used in many different git outputs (see the first paragraph from https://git-scm.com/docs/git-commit#_discussion)
  • as for the text of the commit, it is recommended that it wraps at 72 characters. If not, it makes git log hard to read, among other git output.

Copy link
Contributor

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

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

A couple of things to clarify

Makefile Outdated Show resolved Hide resolved
pkg/v1/builder/template/plugintemplates/Makefile.tmpl Outdated Show resolved Hide resolved
pkg/v1/builder/template/plugintemplates/Makefile.tmpl Outdated Show resolved Hide resolved
@chandrareddyp chandrareddyp force-pushed the topic/cpamuluri/builder-cli-compile-fix branch 7 times, most recently from 94e04f3 to 25e086e Compare September 20, 2022 04:44
@chandrareddyp
Copy link
Contributor Author

build-plugin-local will compile the plugin and put it in the artifacts directory within the repository.

We should add install-plugin-local that installs the plugin as well. Because as a end user I would like to do some change in the plugin, then I want to build as well as install the plugin to test my change.

Implementation has updated to add new make target install-plugin-local to build and update given plugin locally.

Copy link
Contributor

@anujc25 anujc25 left a comment

Choose a reason for hiding this comment

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

One comment. But otherwise looks good.

Makefile Outdated Show resolved Hide resolved
As part of this changes:
-The command 'tanzu builder cli compile --match package --version v0.27.0-dev' is updated to make "--version" flag as optional
-New make target 'build-plugin-local' has added, to build the given plugin locally.
-New make target 'install-plugin-local' has added, to build and install the given plugin locally.
@chandrareddyp chandrareddyp force-pushed the topic/cpamuluri/builder-cli-compile-fix branch from 1b5ad77 to 25d8570 Compare September 20, 2022 17:03
Copy link
Contributor

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks!

Copy link
Contributor

@anujc25 anujc25 left a comment

Choose a reason for hiding this comment

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

Thanks for the change. LGTM

@anujc25 anujc25 added the ok-to-merge PRs should be labelled with this before merging label Sep 21, 2022
@chandrareddyp chandrareddyp merged commit ed3014a into main Sep 21, 2022
@chandrareddyp chandrareddyp deleted the topic/cpamuluri/builder-cli-compile-fix branch September 21, 2022 16:45
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
cla-not-required ok-to-merge PRs should be labelled with this before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants