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
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants