search

sudomesh / makenode

Configures new sudo mesh nodes
8 stars 8 forks source link

Discourage running makenode from master branch #27

Closed bennlich closed 6 years ago

bennlich commented 6 years ago

From a chat with @paidforby several weeks ago, we wanted a way to separate stable versions of makenode from the bleeding-edge latest. This PR tries to address that.

makenode will now refuse to run if it finds it is not on a branch called stable. The user can override this behavior with the --forceVersion option, and a helpful error message is printed if you do try to run makenode from a non-stable branch.

To cut a new makenode release, we would merge master into stable and push.

Please reply w/ feedback @jhpoelen @gobengo @paidforby @sierkje . I like that this could help us avoid configuring new nodes with untested versions of makenode, but am also aware that this adds a some friction to the configuration process.

One alternative would be to avoid committing unstable code to master (and commit it to a branch instead).

If this PR does seem like a good solution, I'll edit the readme before merging.

(And hopefully we can move to zeroconfig soon anyway. But until then...)

gobengo commented 6 years ago

I applaud your work over my opinions, but I must say this seems weird. Generally a piece if software should not have any coupling to a particular version control system. i.e. it should work even if I remove the .git directory or have cloned from a private form with different branching strategy.

Your alternative of keeping the master branch safe is infinitely more common.

One prerequisite that helps with that is to define 'unstable code', and write a program that returns true or false if a given state of the code is 'unstable', ie write a test. Don't accept changes into master that don't pass the test. If code passes the test and gets into master but then is deemed 'unstable', revert the merge, update the test so that it wouldn't have allowed the old bad code, fix the code, and then try again to get it into master.

-1, and again, I applaud your action.

On Fri, Mar 23, 2018, 12:49 AM Benny Lichtner notifications@github.com wrote:

From a chat with @paidforby https://github.com/paidforby several weeks ago, we wanted a way to separate stable versions of makenode from the bleeding-edge latest. This PR tries to address that.

makenode will now refuse to run if it finds it is not on a branch called stable. The user can override this behavior with the --forceVersion option, and a helpful error message is printed if you do try to run makenode from a non-stable branch.

To cut a new makenode release, we would merge master into stable and push.

Please reply w/ feedback @jhpoelen https://github.com/jhpoelen @Juul https://github.com/juul @paidforby https://github.com/paidforby . I like that this could help us avoid configuring new nodes with untested versions of makenode, but am also aware that this adds a some friction to the configuration process.

One alternative would be to avoid committing unstable code to master (and commit it to a branch instead).

If this PR does seem like a good solution, I'll edit the readme before merging.

(And hopefully we can move to zeroconfig soon anyway. But until then...)

You can view, comment on, or merge this pull request online at:

https://github.com/sudomesh/makenode/pull/27 Commit Summary

  • makenode will now refuse to run if not on a branch called 'stable'

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/sudomesh/makenode/pull/27, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKfBiiCP7cURukkTXtJIvy9teIBaBLYks5thDiVgaJpZM4S3900 .

jhpoelen commented 6 years ago

Also, thanks for the initiative and inviting feedback.

The issues with makenode that I have observed over the last year or so, are out-dated dependencies, and issues reaching secrets.sudomesh.org to obtain mesh ip. Neither of these issues would have been prevented with your solution because both issues are caused by things outside the program.

However, if we do encounter stability issues, I'd suggest to use a regular release mechanism: tag a release, describe what changed, and describe how folks get used the stable version instead of the latest. This also implies that we have to figure out what stable means, along the lines of @gobengo . Do we test on all platforms? What kind of tests do we run? Which routers do we include in integration testing?

In summary, I think the proposed solution does not solve a pressing problem. However, if stability is an issue, I'd suggest to use a more conventional and simpler tag/release mechanism.

Hope this helps.

jhpoelen commented 6 years ago

PS - @bennlich I do remember a bug I introduced recently by forgetting to remove some configuration. With your review, I was able to quickly fix the issue. See https://github.com/sudomesh/bugs/issues/23#issuecomment-375550444 . This would have been easily introduced in makenode itself. In this case we saw the issue, fixed it and moved on.

bennlich commented 6 years ago

Thanks @jhpoelen and @gobengo for looking at this. Going to close since it sounds like it would be more useful to start defining tests than to prevent newly committed code from getting tested (which this PR would inadvertently do).

For context, I think when I last talked w/ @paidforby, the stability issue did seem kind of pressing. I think it was because we had just flashed a few nodes w/ a version of makenode that disabled logging.

Both of you mentioned writing tests as a prerequisite, which makes sense to me. Can start with a manual guide, like some of @jhpoelen's patch guides, and then move to a script.

gobengo commented 6 years ago

@bennlich @jhpoelen I made a proposal for stable releases on the wiki of this repo. https://github.com/sudomesh/makenode/wiki/Proposal:-Stable-Releases#proposal-separate-master-branch-from-releases

Tell me what you think. Let's keep using this PR for discussion until in-person sync since wiki doesn't support conversation. I can also make it a straw man PR into a "release-process.md" document.

Have to board a plane, but things I didn't touch on, but in my mind, are:

From what I understand, @jhpoelen work on https://github.com/sudomesh/babeld-lab may be useful to fill in my '??'