Thank you very much Cody for pointing me in the right direction. I already went ahead and followed the Gerrit instructions and created a CR: https://cr.joyent.us/#/c/2353/ <https://cr.joyent.us/#/c/2353/> I did this to learn the process of working with Gerrit (not that the bug/CR itself isn’t important - it is). The workflow is different compared to the GitHub/GitLab (forks/branches) style. I’ve accidentally created and abandoned another CR when pushing an update to gerrit - sorry for that. Then I followed the Gerrit documentation to update the CR and I’ve just noticed the "We have one major exception to the standard Gerrit workflow:…” message in the wiki. I’ve used the “amend + Change-Id” method to add another patchset and again: I’m sorry for that (I didn’t notice it the first time).
Should I abandon the Change 2353 and create a new one? Daniel > On 17 Aug 2017, at 20:51, Cody Mello <[email protected]> wrote: > > 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 >
signature.asc
Description: Message signed with OpenPGP
------------------------------------------- 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
