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

Clean makefile #1532

Merged
merged 4 commits into from Mar 2, 2022
Merged

Clean makefile #1532

merged 4 commits into from Mar 2, 2022

Conversation

SamLR
Copy link
Contributor

@SamLR SamLR commented Feb 25, 2022

Closes: #1477

What changed?

  • Added documentation for the tools used in the makefile
  • Fixed bug due to missing plugins for buf
  • General clean up

Why?

This should improve the developer experience and make understanding various workflows easier.

How did you test it?

As much as possible changes have been tested (albeit only on a mac)

Copy link
Contributor

@dhwthompson dhwthompson left a comment

Choose a reason for hiding this comment

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

I don’t think I’m well-placed to review any of the front-end changes – that world is all still a bit of a mystery to me – but I can happily review the documentation side of things.

README.md Outdated

This is a list of the tools you may need to install:

* [go](go.dev) -- Primary compiler for the CLI.
Copy link
Contributor

Choose a reason for hiding this comment

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

GitHub gets confused when URLs look like relative URLs.

Suggested change
* [go](go.dev) -- Primary compiler for the CLI.
* [go](https://go.dev) -- Primary compiler for the CLI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 7857b90

README.md Outdated
Some other tools are installed automatically by the makefile for you:

* [go-acc](https://github.com/ory/go-acc) -- Calculates code coverage for go.
* [gcov2lcov](github.com/jandelgado/gcov2lcov) -- Converts output from go-acc to a format lcov understands.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* [gcov2lcov](github.com/jandelgado/gcov2lcov) -- Converts output from go-acc to a format lcov understands.
* [gcov2lcov](https://github.com/jandelgado/gcov2lcov) -- Converts output from go-acc to a format lcov understands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 7857b90

README.md Outdated

* [go-acc](https://github.com/ory/go-acc) -- Calculates code coverage for go.
* [gcov2lcov](github.com/jandelgado/gcov2lcov) -- Converts output from go-acc to a format lcov understands.
* [controller-gen](sigs.k8s.io/controller-tools/cmd/controller-gen) -- Helps generate kubernetes controller code.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* [controller-gen](sigs.k8s.io/controller-tools/cmd/controller-gen) -- Helps generate kubernetes controller code.
* [controller-gen](https://sigs.k8s.io/controller-tools/cmd/controller-gen) -- Helps generate kubernetes controller code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 7857b90

@@ -75,6 +75,33 @@ To set up a development environment for the CLI
6. Start the in-cluster API replacement job (powered by [http://tilt.dev](tilt.dev)) with `make cluster-dev`
7. make or make unit-tests to ensure everything built correctly.

### Requirements/tools
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉👍🧁 for extra documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Danke

ozamosi pushed a commit that referenced this pull request Feb 28, 2022
This just calls "flux" instead of a specific path - it has the neat
side-effect that instead of using WEAVE_GITOPS_FLUX_BIN_PATH, you can
just set your own flux bin path using the PATH variable.

Right now, there are only two things we do with this flux: creating a
secret, and wrapping `flux check` in `gitops check`.

The gitops check should end up removing the flux check.

The secret is part of accessing private git repos, which is called by
auth which is called by both upgrade and profiles.

There's still a flux installed by `make dependencies` - I've adapted
the auth tests to use the one installed by that, but as it's not added
to your path, it's not going to be used by the local CLI. Flux is now
a dependency I expect you to install yourself (will not document right
now, but note to self to add to the documentation #1532 is adding).
ozamosi pushed a commit that referenced this pull request Feb 28, 2022
This means that from now on, there's one allowed version of the buf
toolchain, however that toolchain should be installed by just running
`make proto`.

I stole this from #1532, and extended it with the buf command itself
as well.
ozamosi pushed a commit that referenced this pull request Feb 28, 2022
This means that from now on, there's one allowed version of the buf
toolchain, however that toolchain should be installed by just running
`make proto`.

I stole this from #1532, and extended it with the buf command itself
as well.

This also upgrades grpc, in order to support buf 1.0.
Sam Cook added 4 commits March 2, 2022 11:44
This should surpress warnings when building UI components without go
installed.
The `cmd/gitops/ui/run/dist/{,index.html}` targets create an empty
directory and file respectively and are only used in github workflows to
avoid git errors due to missing embed targets. This removes them in
favour of making it clear in the github actions that nothing is actually
being made.

This also re-orders the dependencies for the ui-lib target within the
makefile to make it clearer what lives with what
We should be clear about the tools used to build gitops
Some common patterns in the github workflows is to either create a fake
ui dist or flux file (`mkdir ... && touch ...`) or to run `npm ci` then
separately generate the ui. The former can now be removed as the
directories to be embedded have had keep files added while the latter
can be replaced with the simple `make ui` which performs all necessary
steps to generate the ui.
@SamLR SamLR merged commit fcaf2eb into v2 Mar 2, 2022
@SamLR SamLR deleted the clean-makefile branch March 2, 2022 12:10
ozamosi pushed a commit that referenced this pull request Mar 3, 2022
This just calls "flux" instead of a specific path - it has the neat
side-effect that instead of using WEAVE_GITOPS_FLUX_BIN_PATH, you can
just set your own flux bin path using the PATH variable.

Right now, there are only two things we do with this flux: creating a
secret, and wrapping `flux check` in `gitops check`.

The gitops check should end up removing the flux check.

The secret is part of accessing private git repos, which is called by
auth which is called by both upgrade and profiles.

There's still a flux installed by `make dependencies` - I've adapted
the auth tests to use the one installed by that, but as it's not added
to your path, it's not going to be used by the local CLI. Flux is now
a dependency I expect you to install yourself (will not document right
now, but note to self to add to the documentation #1532 is adding).
jpellizzari pushed a commit that referenced this pull request Mar 3, 2022
This means that from now on, there's one allowed version of the buf
toolchain, however that toolchain should be installed by just running
`make proto`.

I stole this from #1532, and extended it with the buf command itself
as well.

This also upgrades grpc, in order to support buf 1.0.
ozamosi pushed a commit that referenced this pull request Mar 7, 2022
This just calls "flux" instead of a specific path - it has the neat
side-effect that instead of using WEAVE_GITOPS_FLUX_BIN_PATH, you can
just set your own flux bin path using the PATH variable.

Right now, there are only two things we do with this flux: creating a
secret, and wrapping `flux check` in `gitops check`.

The gitops check should end up removing the flux check.

The secret is part of accessing private git repos, which is called by
auth which is called by both upgrade and profiles.

There's still a flux installed by `make dependencies` - I've adapted
the auth tests to use the one installed by that, but as it's not added
to your path, it's not going to be used by the local CLI. Flux is now
a dependency I expect you to install yourself (will not document right
now, but note to self to add to the documentation #1532 is adding).
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.

None yet

3 participants