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

Fix Vet Errors #2153

Merged
merged 5 commits into from
Jun 16, 2022
Merged

Fix Vet Errors #2153

merged 5 commits into from
Jun 16, 2022

Conversation

tchaudhry91
Copy link
Member

@tchaudhry91 tchaudhry91 commented Jun 12, 2022

What does this change

This PR is meant to eliminate the go vet warnings/errors in the codebase.
Present State:
1 big commit which fixes the most common recurring vet errors (unkeyed fields in composite literals).
There are 2 major culprits here:
ExtendedBundle which has an embedded Bundle struct. Relatively straightforward fix here to add Bundle before supplying the embedded struct. Even I didn't know this was a thing, always thought embedded structs had to be anonymous :)
Found the solution from here: https://stackoverflow.com/questions/49461354/go-vet-composite-literal-uses-unkeyed-fields-with-embedded-types

The other one is in the Primitive.E type in the mongo driver. We are generally passing Key and Value as a {"some_key", "some_value"} in all places. The recommended way is to go with {Key: "some_key", Value: "some_value"}. This feels a little extra verbose to me, but as highlighted in the link below, it offers good protection against changes in the library, so I suggest we keep it. Also fixes vet warnings :)
https://stackoverflow.com/questions/54548441/composite-literal-uses-unkeyed-fields

Update:
I have added another commit to resolve all the Struct Tag issues. Only minor typos here (although, it's surprising functionality wasn't broken in a few places). Nevertheless, all resolved now.

Update 2:
I have also renamed the example tests to use suffixes according to (https://pkg.go.dev/testing#hdr-Examples).
The old values were incompatible with Vet.

What issue does it fix

Makes incremental progress on #2081

Notes for the reviewer

This is a major chunk of the go vet warnings, but not all. I will resolve the rest via a separate PR. I kept this one segregated because of the theme.
Update: Also added a commit to resolve struct tag issues.

Checklist

  • Did you write tests?
  • Did you write documentation?
  • Did you change porter.yaml or a storage document record? Update the corresponding schema file.
  • If this is your first pull request, please add your name to the bottom of our Contributors list. Thank you for making Porter better! 🙇‍♀️

Reviewer Checklist

  • Comment with /azp run test-porter-release if a magefile or build script was modified
  • Comment with /azp run porter-integration if it's a non-trivial PR

Signed-off-by: Tanmay Chaudhry <tanmay.chaudhry@gmail.com>
Signed-off-by: Tanmay Chaudhry <tanmay.chaudhry@gmail.com>
@tchaudhry91 tchaudhry91 changed the title Fix vet errors for unkeyed fields in composite literals Fix Vet Errors Jun 13, 2022
@tchaudhry91
Copy link
Member Author

I have added a comment regarding the broken unit-test. The malformed struct tag for yaml meant the file wasn't using the correct name. Will defer to someone with more knowledge to get the best way to fix this. Long term, we probably want to keep the correct tag and regenerate the golden comparison file. Not sure how much that will impact current consumers. Alternatively, we could use the "wrong" value in the tag if we want. Let me know.

@VinozzZ
Copy link
Contributor

VinozzZ commented Jun 13, 2022

Thank you for submitting the PR to fix lint and vet errors!

I have added a comment regarding the broken unit-test. The malformed struct tag for yaml meant the file wasn't using the correct name. Will defer to someone with more knowledge to get the best way to fix this. Long term, we probably want to keep the correct tag and regenerate the golden comparison file. Not sure how much that will impact current consumers. Alternatively, we could use the "wrong" value in the tag if we want. Let me know.

Thank you for putting up the PR for fixing lint and vet errors. From my perspective,, we should fix all the schema issues while we are still in alpha. but we probably should wait for @carolynvs to confirm it

Signed-off-by: Tanmay Chaudhry <tanmay.chaudhry@gmail.com>
Signed-off-by: Tanmay Chaudhry <tanmay.chaudhry@gmail.com>
@tchaudhry91
Copy link
Member Author

tchaudhry91 commented Jun 15, 2022

Hey all,
This PR is now complete from my point of view, here is a summary for the reviewers:

  1. Fixes all the unkeyed fields in the composite literals.
  2. Fixes the struct tags. This is a bit more complicated as this will technically be a breaking change? This is because the old tags were incorrect and hence ignored. So far, this has been detected in one place in a test file, but I assume others would also be impacted. For now, I have regenerated that test file and the tests pass now. If we want to go with another approach, let me know.
  3. Renamed ExampleTests to use PackageName_suffix format as dictated by Vet.

The unit test failure in the PR seems to be due to a connection reset, i.e flaky. Can someone re-run it? I have confirmed that unit tests run fine on multiple machines.

Copy link
Member

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

Thank you for going over the codebase and getting all the vet errors fixed! Let me know if you'd like to add that constructor function and I'll hold off on merging.

@@ -196,7 +196,7 @@ func TestStoreManifest(t *testing.T) {
}{
{
name: "embedded manifest",
bundle: cnab.ExtendedBundle{bundle.Bundle{
bundle: cnab.ExtendedBundle{Bundle: bundle.Bundle{
Copy link
Member

Choose a reason for hiding this comment

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

We can always switch to a shorter constructor function, e.g. cnab.NewBundle(b).

I think we use extended bundles enough that this would be worthwhile to change. If you are up for it, it would be great to have that in this PR, but if you are too busy, we can do it in a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good! I'll make this change.

pkg/porter/list.go Show resolved Hide resolved
@@ -41,7 +41,7 @@ func TestRoundTripDataOverGRPC(t *testing.T) {
// Add an index to support filtering
const collection = "things"
err = client.EnsureIndex(ctx, plugins.EnsureIndexOptions{Indices: []plugins.Index{
{Collection: collection, Keys: bson.D{{"namespace", 1}, {"name", 1}}},
{Collection: collection, Keys: bson.D{{Key: "namespace", Value: 1}, {Key: "name", Value: 1}}},
Copy link
Member

Choose a reason for hiding this comment

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

Since we don't control the bson package, I don't think making a constructor function is necessary and using explicit keys when initializing E isn't too big of a deal.

Thanks for tracking this down!

@@ -200,7 +200,7 @@ type InstallationStatus struct {

// Uninstalled indicates if the installation has successfully completed the uninstall action.
// Once that state is reached, Porter should not allow further stateful actions.
Uninstalled *time.Time `json:"uninstalled" yaml"uninstalled" toml:"uninstalled"`
Uninstalled *time.Time `json:"uninstalled" yaml:"uninstalled" toml:"uninstalled"`
Copy link
Member

Choose a reason for hiding this comment

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

This is why I'm super excited to have vet run by default soon. It will save me from myself 😝

@carolynvs
Copy link
Member

I restarted the test run, it was a temporary glitch in the matrix. 😀

@tchaudhry91
Copy link
Member Author

@carolynvs I'll add the constructor in this PR only. Let's hold off on merging till then. Shouldn't be long

Signed-off-by: Tanmay Chaudhry <tanmay.chaudhry@gmail.com>
@tchaudhry91
Copy link
Member Author

tchaudhry91 commented Jun 16, 2022

@carolynvs I have added the constructor and made the replacements in all the non-zero initializations.
For consistency it's probably better to come up with a something to replace all the ExtendedBundle{} symbols too. Although, these are almost exclusively used to return zero values along with an error, in which case this value should never be referenced, however, for readability, I suppose a mix of NewBundle and ExtendedBundle{} might be a bit confusing. Nevertheless, if we want to replace those too, I can do in a new PR. This one is getting a bit unwieldy already :)

Copy link
Member

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

This looks great, thanks!

I don't have a problem with using cnab.NewBundle when wrapping and cnab.ExtendedBundle{} when we want an empty bundle. 👍

@carolynvs carolynvs merged commit 0c3d0b6 into getporter:release/v1 Jun 16, 2022
joshuabezaleel pushed a commit to joshuabezaleel/porter that referenced this pull request Jun 23, 2022
* Fix lint errors for unkeyed fields in composite literals

Signed-off-by: Tanmay Chaudhry <tanmay.chaudhry@gmail.com>

* resolve lint errors on tags

Signed-off-by: Tanmay Chaudhry <tanmay.chaudhry@gmail.com>

* Updated golden file to account for bad struct tag fix

Signed-off-by: Tanmay Chaudhry <tanmay.chaudhry@gmail.com>

* Vet Fix: Rename example tests to use suffixes.

Signed-off-by: Tanmay Chaudhry <tanmay.chaudhry@gmail.com>

* Replace ExtendedBundle{} initialization with a NewBundle constructor

Signed-off-by: Tanmay Chaudhry <tanmay.chaudhry@gmail.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>
carolynvs added a commit that referenced this pull request Jun 23, 2022
* Use user specified directory for resolving file path (#2142)

* use user specified build directory if provided for porter manifest

Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>

* update tests

Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>

* update doc and fix tests

Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>

* address comment

Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>

* explicitly set default value for o.Dir

Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>

* clearer help text

Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

* Update to helm3 mixin v0.1.16

v0.1.16 includes fixes for using nonroot invocation images

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

* Sanitize archive folder name (#2154)

* fix archive folder creation

Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>

* replace path separator instead

Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>

* modify test

Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

* Adding pagination for installation, parameter, and credential list result using skip and limit option (#2137)

* Add pagination option for installation list command using skip and limit flag

Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

* Increase plugin start/stop timeouts

As I was adding back in net/rpc plugins (the legacy v0 plugins), I
realized that our plugin timeouts don't work well for net/rpc since it
is much slower than gRPC.

I've bumped both the plugin start and stop timeout defaults to make it
less likely that a user will run into the timeout, while still giving us
a good "oops the plugin is broken" timeout detection.

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

* Add InstallationStore.FindInstallations (#2119)

The advanced dependencies proposal needs to be able to search for
installations based on more complex critieria than is available in the
ListInstallations function (which is intended to support the porter
installation list command). FindInstallations lets us craft any valid
mongodb find query and execute it, returning a list of installations.

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

* Rename DisplayRun.ClaimID to ID

I missed this field when I did a sweep earlier to remove the use of the
word claim in the release/v1 branch. In the rest of the CLI's output we
call the run's id just ID or RunID, and should be consistent with that.

I've changed DisplayID.ClaimID to ID so that we aren't exposing the term
claim to our users (and it's not really the claim id anymore anyway).

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

* Support Docker TLS environment variables

We are using the docker cli library to build images and I had thought
this gave us automatic support for building against a remote docker
host. It works fine for DOCKER_HOST, but turns out the TLS configuration
environment variables are only parsed when the docker CLI flags are
bound (which doesn't occur when we use it as a library).

I've updated how we initialize the docker cli library so that
DOCKER_TLS_VERIFY and DOCKER_CERT_PATH are picked up and passed to the
library.

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

* Add vet and lint targets to magefile

Signed-off-by: Tanmay Chaudhry <tanmay.chaudhry@gmail.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

* Add ListOption input parameter struct and enable skip and limit option to credential and parameter list command as well

Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

* Leave out default value for ListOption's properties

Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

* Remove commented function signature

Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

* Convert CreateListFilter to ToFindOptions method for ListOptions type receiver

Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

Co-authored-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Co-authored-by: Tanmay Chaudhry <tanmay.chaudhry@gmail.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

* Add state and status to list installation

Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

* fix archive folder test

Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

* Fix Vet Errors (#2153)

* Fix lint errors for unkeyed fields in composite literals

Signed-off-by: Tanmay Chaudhry <tanmay.chaudhry@gmail.com>

* resolve lint errors on tags

Signed-off-by: Tanmay Chaudhry <tanmay.chaudhry@gmail.com>

* Updated golden file to account for bad struct tag fix

Signed-off-by: Tanmay Chaudhry <tanmay.chaudhry@gmail.com>

* Vet Fix: Rename example tests to use suffixes.

Signed-off-by: Tanmay Chaudhry <tanmay.chaudhry@gmail.com>

* Replace ExtendedBundle{} initialization with a NewBundle constructor

Signed-off-by: Tanmay Chaudhry <tanmay.chaudhry@gmail.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

* Improve error message loading wrong schema (#2157)

* Improve error message loading wrong schema

Signed-off-by: Kevin Barbour <kevinbarbourd@gmail.com>

* Add myself to CONTRIBUTORS.MD

Signed-off-by: Kevin Barbour <kevinbarbourd@gmail.com>

* Don't use errors pkg, fix assert in test

Signed-off-by: Kevin Barbour <kevinbarbourd@gmail.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

* Add prow github action

This adds a prow github action that allows specified people (in the
OWNERS file) to comment on a pull request with /lgtm to review the pull
request, or /approve to merge the pull request.

The github action handles executing the commands for you so that you
don't need to have maintainer rights on the repository.

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

* Switch prow to use pull_request instead of _target

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

* Updated installation schema with correct dependency schema

Signed-off-by: Steven Gettys <s.gettys@f5.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

* changed new manifest description for test

Signed-off-by: Steven Gettys <s.gettys@f5.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

* Update k8s and containerd dependencies

* Update to cnab-go v0.23.4
* Update containerd to v1.6.6
* Updated k8s to v0.24.1.

This does not update docker since buildkit uses a funny unreleased
version of docker. We won't be able to update to a new version of Docker
until there's a release that has the new feature that buildkit relies
upon. See go.mod for a link to the troublesome package in question.

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

* Add comments

Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

* StateDefined as default value

Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

* Move displayinstallation's state and status to metadata

Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

* Add golden file test for print installation

Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

* Add unit test for displayInstallation's state and status

Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

* Change function name from set to get

Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

* Revert changes on test file

Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

* add new line

Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

* resolve conflict

Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

* fix comment

Signed-off-by: joshuabezaleel <joshua.bezaleel@gmail.com>

Co-authored-by: Yingrong Zhao <yingrong.zhao@gmail.com>
Co-authored-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Co-authored-by: Tanmay Chaudhry <tanmay.chaudhry@gmail.com>
Co-authored-by: Kevin Barbour <kevinbarbourd@gmail.com>
Co-authored-by: Steven Gettys <s.gettys@f5.com>
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