Skip to content

Conversation

@griznog
Copy link
Contributor

@griznog griznog commented May 25, 2020

Allow the use of YAML list syntax when defining node groups.

 rack5: - 'example[200-205]' # some comment about example[200-205] - 'example245' - 'example300,example[401-406]' griznog$ nodeset -f @racks:rack5 example[200-205,245,300,401-406]
@degremont degremont self-requested a review May 26, 2020 08:45
Copy link
Collaborator

@degremont degremont left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your patch. It looks good, could you also:

  • Add a test for that in tests/NodeSetGroupTest.py, at the end of YAMLGroupLoaderTest class
  • A tiny example in the doc in doc/sphinx/config.rst

Could you also edit your commit message and make it looks similar to 9b3a239 by example, meaning:

  • A title with a prefix : NodeUtils:
  • A small title (less than 70 char): allow YAML list to declare node groups
  • A blank line
  • A sentence explaining the change more in detail
@degremont degremont requested a review from thiell May 26, 2020 08:51
@degremont degremont added this to the 1.8.4 milestone May 26, 2020
@griznog griznog changed the title Small change to allow use of YAML list syntax for nodesets NodeUtils: allow YAML list to declare node groups May 26, 2020
@degremont
Copy link
Collaborator

Thanks for the changes. They looks good! However, when I was talking about the commit message, I was really talking about the commit message of the unique commit you should have in this branch.
Could you squash all your 4 commits together in 1 and update the commit message as explained previously?

griznog added 2 commits May 27, 2020 15:34
If group is a list, ','.join to make a string before returning _list_node().
@griznog
Copy link
Contributor Author

griznog commented May 27, 2020

Clearly I'm lost with the git squash part of this, I don't understand why I wound up with the second merge commit? Lots of "first time doing X" for me in this process, thanks for being patient.

Copy link
Collaborator

@thiell thiell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey thanks @griznog! Getting ready for your cluster of tractors? ;-)
We'll be able to squash the commits for you when merging, no worries.

@degremont degremont merged commit a0803f4 into cea-hpc:master May 28, 2020
@degremont
Copy link
Collaborator

Thanks @griznog , I managed the squash things. Thanks for your patch!
If you want to know more about squashing, for next time ;) : https://stackoverflow.com/questions/5189560/squash-my-last-x-commits-together-using-git

@griznog
Copy link
Contributor Author

griznog commented May 28, 2020

Hey thanks @griznog! Getting ready for your cluster of tractors? ;-)
We'll be able to squash the commits for you when merging, no worries.

I'm hoping to keep my squash in the garden :)

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