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/Cleanup Makefile - Closes kubeshop/kusk-gateway#418 #419

Merged
merged 2 commits into from
May 25, 2022

Conversation

mbana
Copy link
Contributor

@mbana mbana commented May 18, 2022

Fix/Cleanup Makefile - Closes #418:

  • Replace go get with go install in Makefile.
  • Tidy up Makefile.
  • Introduce tools in Makefile that downloads all the tools/dependencies required.
  • DOCKER_BUILDKIT: No point in defining this everyone - exporting and defining should be enough.

For further information, see:


NB:

Please try out the branch and tell me if you see any issues on Mac. I normally run:

git pull && git checkout mbana-makefile-fixes && make create-env

Which runs to completion.


This PR...

Changes

Fixes

Checklist

  • tested locally
  • added new dependencies
  • updated the docs
  • added a test

@netlify
Copy link

netlify bot commented May 18, 2022

Deploy Preview for kusk-docs-preview ready!

Name Link
🔨 Latest commit 85539c8
🔍 Latest deploy log https://app.netlify.com/sites/kusk-docs-preview/deploys/628cec2a8a61c60008ace635
😎 Deploy Preview https://deploy-preview-419--kusk-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

jasmingacic
jasmingacic previously approved these changes May 18, 2022
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Copy link
Contributor

@kylehodgetts kylehodgetts left a comment

Choose a reason for hiding this comment

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

A few comments made above

Do you think it's also worth adding a test into the CI to make sure this doesn't break again unexpectedly?

@mbana
Copy link
Contributor Author

mbana commented May 18, 2022

A few comments made above

Do you think it's also worth adding a test into the CI to make sure this doesn't break again unexpectedly?

Thanks.

Yes.

What particular type of tests do you have in mind?

In a separate PR or this one?

Actually, the reason I wanted folks to try out my branch was to be sure I hadn't broken anything. I am going to test further before we even get to merge state.

@jasmingacic
Copy link
Contributor

If possible add consistent test even though CI should detect it

@kylehodgetts
Copy link
Contributor

A separate PR.

on build before merge to master

  • check that make works, i.e. tools are found and installed properly in kusk-gateway/bin
  • check the generating code works - protoc, kustomize etc

@jasmingacic
Copy link
Contributor

works with go 1.18 both on MacOS and Ubuntu
Ship it

mbana and others added 2 commits May 24, 2022 15:30
* Replace `go get` with `go install` in `Makefile`.
* Tidy up `Makefile`.
* Introduce `tools` in `Makefile` that downloads all the tools/dependencies required.
* `DOCKER_BUILDKIT`: No point in defining this everyone - exporting and defining should be enough.

For further information, see:

* kubernetes-sigs/kubebuilder#2566
* kubernetes-sigs/kubebuilder#2486
1. > The indentation for these variable assignments are a bit all over the place, can you align them, please?
2. > If you've tested that kustomize v4 works as expected, this comment can be removed

Closes #418
@mbana mbana requested review from kylehodgetts and jasmingacic and removed request for Tarick May 24, 2022 14:32
Copy link
Contributor

@jasmingacic jasmingacic left a comment

Choose a reason for hiding this comment

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

Looking good

@mbana mbana requested a review from jasmingacic May 25, 2022 15:25
@mbana mbana self-assigned this May 25, 2022
@mbana mbana merged commit 42e0b90 into main May 25, 2022
@mbana mbana deleted the mbana-makefile-fixes branch May 25, 2022 15:38
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.

Makefile is broken!!!
3 participants