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 Makefile (failing linters disabled) #301

Merged

Conversation

c0d1ngm0nk3y
Copy link
Contributor

fixes #260
first chunk of #293

Summary

This pr adds a Makefile and also fixes some linting errors so that make lint will run without error.

With the exception that some checks had been deactivated, because the errors were beyond the scope of this.

Use Cases

It's standard in a lot of Go projects. It also makes it easier to run test, lint, and CI.

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

@c0d1ngm0nk3y c0d1ngm0nk3y requested a review from a team as a code owner December 12, 2023 15:30
@c0d1ngm0nk3y c0d1ngm0nk3y added semver:patch A change requiring a patch version bump type:task A general task labels Dec 12, 2023
@anthonydahanne
Copy link
Member

@pivotal-david-osullivan , @dmikusa , are we settled on this way of running linting?
Personally, I'm fine with it, even if I'm still not a big fan of Makefile in general

@dmikusa
Copy link
Contributor

dmikusa commented Dec 23, 2023

@anthonydahanne I think that's a fair question, and worth taking a step back and reviewing.

Here's what I can share.

  1. I've been told that using a Makefile is somewhat common in Go projects.
  2. We use a Makefile in libcnb and it's used in other CNB projects too. There's a template there in terms of commands that we could follow and get some parity with CNB.
  3. I don't have any particular fondness for Makefile, the reason I created the issue to do this is because 1.) and 2.).
  4. If folks are not familiar with Makefile, it's probably not a big deal cause the complexity of what's done there is low.
  5. The other buildpacks team just uses pure shell scripts for builds. No Makefiles as far as I know. If we were to not go with Makefile, then using their build scripts would make the most sense.
  6. I run the Go commands to test/build and it works out well too. There are not that many and they are not that complicated, so I suppose we could just do nothing too. This isn't something that we get a ton of complaints about either.

@anthonydahanne and @c0d1ngm0nk3y - Having said all that, I'm open to suggestions. Thoughts?

@dmikusa
Copy link
Contributor

dmikusa commented Dec 23, 2023

@c0d1ngm0nk3y One thing I noticed, that would probably be good to do in this PR, is to remove Richgo. The author no longer recommends using it, so I suspect that it could break or mess up our pipelines. https://github.com/kyoh86/richgo?tab=readme-ov-file#notice-what-i-think-about-richgo

@c0d1ngm0nk3y
Copy link
Contributor Author

One thing I noticed, that would probably be good to do in this PR, is to remove Richgo.

Sure. I can do this. I just wonder because it is used all over the place (e. libcnb, lifecycle).

@c0d1ngm0nk3y
Copy link
Contributor Author

Personally, I'm fine with it, even if I'm still not a big fan of Makefile in general

What is your preferred way? I think it is easy, quite common and I find it very refreshing if related projects (e.g. libcnb, lifecycle) how something in common.

Copy link

linux-foundation-easycla bot commented Feb 2, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: c0d1ngm0nk3y / name: Ralf Pannemans (9ef2865)

Signed-off-by: Ralf Pannemans <ralf.pannemans@sap.com>
@c0d1ngm0nk3y
Copy link
Contributor Author

btw: When rebase, I had to comment out just another linter because of issues. So there is a real benefit in having a linter running in the pr validation.

@c0d1ngm0nk3y
Copy link
Contributor Author

  • I've been told that using a Makefile is somewhat common in Go projects.

That is my impression as well.

  • We use a Makefile in libcnb and it's used in other CNB projects too. There's a template there in terms of commands that we could follow and get some parity with CNB.

That is for me the killer argument. It feels very strange to switch between CNB project and out of the sudden things are done differently.

  • I don't have any particular fondness for Makefile, the reason I created the issue to do this is because 1.) and 2.).

What would be the alternative? tbh, I do not see a single benefit in doing something special for this project.

  • If folks are not familiar with Makefile, it's probably not a big deal cause the complexity of what's done there is low.

Exactly, the complexity is low. So getting familiar with this Makefile is not too hard, imho.

  • The other buildpacks team just uses pure shell scripts for builds. No Makefiles as far as I know. If we were to not go with Makefile, then using their build scripts would make the most sense.

But libcnb and lifecycle do use a Makefile.

  • I run the Go commands to test/build and it works out well too. There are not that many and they are not that complicated, so I suppose we could just do nothing too. This isn't something that we get a ton of complaints about either.

But running a linter in pr validation is long overdue, isn't it?

@dmikusa
Copy link
Contributor

dmikusa commented Feb 2, 2024

  • I don't have any particular fondness for Makefile, the reason I created the issue to do this is because 1.) and 2.).

What would be the alternative? tbh, I do not see a single benefit in doing something special for this project.

I don't really have strong opinions on this. I mostly said this in case others have strong opinions, to encourage them to speak up. You make good arguments for Makefile, so happy to go along with that.

  • I run the Go commands to test/build and it works out well too. There are not that many and they are not that complicated, so I suppose we could just do nothing too. This isn't something that we get a ton of complaints about either.

But running a linter in pr validation is long overdue, isn't it?

Agreed. So long as it's not making busy work for the project. We've got plenty of things to do, don't need more work :) It's probably a good target for the v2's, to have clean linted code.

@stefanlay
Copy link

/easycla

1 similar comment
@jarias-lfx
Copy link

/easycla

@WillsonHG
Copy link

/easycla

1 similar comment
@mlehotskylf
Copy link

/easycla

@dmikusa
Copy link
Contributor

dmikusa commented Feb 6, 2024

Looks good to me. This is a starting place and we can hone things in as we go.

@dmikusa dmikusa merged commit 54cd048 into paketo-buildpacks:release-2.x Feb 6, 2024
4 checks passed
@loewenstein loewenstein mentioned this pull request May 4, 2024
@dmikusa dmikusa mentioned this pull request May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch A change requiring a patch version bump type:task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants