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

Merge/cleanup Procfiles #15778

Closed
serathius opened this issue Apr 25, 2023 · 11 comments · Fixed by #16040
Closed

Merge/cleanup Procfiles #15778

serathius opened this issue Apr 25, 2023 · 11 comments · Fixed by #16040

Comments

@serathius
Copy link
Member

What would you like to be added?

Procfiles are nice and all, but why do we need to have 3 in root directory Procfile, Procfile.learner, Procfile.v2.

I propose one of three things:

  • Merge/remove Procfile.learner and Procfile.v2 so only Procfile is left.
  • Move procfiles to Documentation/contributor-guide or similar place for contributors.
  • Remove after confirming they are not used by any contributor.

Why is this needed?

Root directory should be minimal to improve readability and avoid overwhelming new contributors.
It's not clear which one of them they should use, nor where it is documented.

@sharathsivakumar
Copy link
Contributor

@serathius and @jmhbnz I would like to work on this.

@serathius
Copy link
Member Author

Let's discuss which option we should do. I don't use procfile as much so I don't want to pick myself.

@sharathsivakumar
Copy link
Contributor

Cool. I am doing a bit of digging around. I will get back to you soon and we can go ahead. In the meantime, do you think it might be a good idea to ask around in the etcd channel if other contributors have any opinions on this as well?

@serathius
Copy link
Member Author

Sure, cc @ahrtr @ptabor @jmhbnz @chaochn47 @lavacat

@ahrtr
Copy link
Member

ahrtr commented Apr 28, 2023

Profile is super useful to launch a local etcd cluster in seconds, let's keep it as it's.

  • Remove Procfile.v2, since we only support zap for the structure log starting from 3.5. And 3.4 also support zap.
  • Append the content of Profile.learner to Profile as comment.

@jmhbnz
Copy link
Member

jmhbnz commented Apr 28, 2023

Suggested approach from @ahrtr sounds good to me. That would cut two files out of root directory which is a great win.

@sharathsivakumar
Copy link
Contributor

Based on the inputs provided by everyone, I will make the changes and create a PR.

@jmhbnz
Copy link
Member

jmhbnz commented Jun 7, 2023

Hey @sharathsivakumar are you still able to work on this? If not that is ok just let us know so we can reassign it. If you're stuck or have any questions please reach out to me on kubernetes slack or reply here. Thanks! 🙏🏻

@shivanshuraj1333
Copy link
Contributor

@jmhbnz this is a small one, I've opened up a PR, if you've time, kindly review.
Assign this issue to me if needed.
Thanks!

@jmhbnz
Copy link
Member

jmhbnz commented Jun 9, 2023

Thanks for your pull request @shivanshu1333, I've reassigned the issue to you.

This issue was originally assigned to another contributor, so in future from a contributor experience perspective my preference would be for you to check in with the assigned contributor first and ask for them to release the issue before raising a pr.

@shivanshuraj1333
Copy link
Contributor

Thanks @jmhbnz and apologies, suggestion noted!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

5 participants