Hi Daniel, On Wed, Aug 9, 2017 at 12:33 AM, Daniel Kontsek <[email protected]> wrote: > AFAIK we should open merge requests here: https://cr.joyent.us/ and not on > GitHub, but we should create an issue on GitHub first, is this correct, or can > we just create CRs in Gerrit?
Yes, merge requests should go through Gerrit for code review. You can find instructions on how to get started using Gerrit here: https://github.com/joyent/joyent-gerrit/tree/master/docs/user You should create an issue on GitHub under the appropriate repository, and then reference it in the commit message. For example, something like: joyent/smartos-live#12345 This Is The Issue's Subject Line Reviewed by: Joe Smith <[email protected]> Approved by: Jane Doe <[email protected]> If you know of an open Joyent JIRA ticket (you can see them at http://smartos.org/bugview/) then you're welcome to reference that in the commit message instead. Once you have a change on Gerrit, you can ping people in #smartos or the Github ticket about it to find reviewers. > Shell scripts coding style (mainly svc/methods and prompt_config) is a > problem. We are seeing mixing of bash coding patterns, even in scripts where > new bash features are used. (e.g. $var vs ${var} vs "${var}", `` vs $(), [ vs > [[). I assume lots of these are just Solaris heritage, but some scripts are > new and yet we see these strange inconsistencies. I'm not going to argue about > line length and tabs vs spaces (although please don't mix them). But as we are > saying: at least do it consistently wrong :) We are happily using shellcheck > [2] for most of our bash scripts and it does solve these kind of problems. Is > there a coding style guide for shell scripts? This is the bash style guide, but it could definitely do with some additional recommendations: https://github.com/joyent/eng/blob/master/docs/index.md#bash-programming-guidelines Dave Eddy at Joyent wrote some personal recommendations for writing shell scripts, many of which should probably also be in the guide: http://daveeddy.com/bash/ A lot of the scripts in smartos-live have vastly different styles. If you're making changes in an area where someone has diverged from the prevailing style of the file, then please do update that region to match, so that over time each file can at least become consistent with itself. In the long run, I'd like to flesh out the style guide and write a linter/style checker to help enforce the guidelines. > For example: we would love to rewrite the smartos_prompt_config.sh script so > it does not use global variables. Would you accept such change? smartos_prompt_config.sh could definitely do with some love. If you can clean it up to be more readable and safer, we'd probably take those changes. Just please make sure to also do testing for all of the paths you change, and attach testing notes to the Github issue. - Cody ------------------------------------------- smartos-discuss Archives: https://www.listbox.com/member/archive/184463/=now RSS Feed: https://www.listbox.com/member/archive/rss/184463/25769125-55cfbc00 Modify Your Subscription: https://www.listbox.com/member/?member_id=25769125&id_secret=25769125-7688e9fb Powered by Listbox: http://www.listbox.com
