search

amendlik / salt-gen-resource

Generate Rundeck node resources from the Salt Mine
Other
8 stars 7 forks source link

Support attributes from grains with nested items #3

Closed Mapel88 closed 5 years ago

Mapel88 commented 5 years ago

select first item by default and raise exception if grain is not a list

amendlik commented 5 years ago

Thank you for the PR. I have a couple of suggestions on this:

  1. Your change assumes the list will contain strings. It will need to account for numeric types and other, nested iterables.
  2. Make sure you are using 4 spaces for indentation. Line 355 contains a tab.
  3. When fixing typos, please rebase and squash those commits into the substantial ones. You can do a force-push to update the PR after rebasing.

Please let me know if you need help with any of the above.

Mapel88 commented 5 years ago
  1. OK. I can add the check you did originally (line 352 & 353)
  2. No problem. I will replace it with spaces.
  3. Sorry. Don't know what's "rebase and squash" and "force push"..
amendlik commented 5 years ago

The whitespace looks good now, and I'm happy to help with the Git magic.

First, there is no need to create a new pull request. If you look at the fine print in this PR, you'll see this instruction:

Add more commits by pushing to the develop branch on Mapel88/salt-gen-resource.

So, this PR can be updated by just pushing new work to the existing remote branch. In fact, you'll see that your latest commit 4a7699b was added to this PR when you pushed it. I'll close out #4 since we don't need it.

Next, let's look at how to clean up typos and other non-functional changes. For this you can use an "interactive" rebase. Here are the steps:

  1. Checkout your develop branch.

  2. Run git rebase -i v1.4. This tells Git, "I want to rewrite some of the history between v1.4 and HEAD."

  3. It will open up an editor with a list of commits from v1.4 to the latest one (HEAD).

    pick 70b08c9 Support attributes from grains with nested items
pick 4294d8b Support attributes from grains with nested items
pick 4a7699b Support attributes from grains with nested items

    The order here is oldest-first, so 70b08c9 was your first commit.

  4. Change the word pick to fixup for the commits that "fix up" an earlier commit.

    pick 70b08c9 Support attributes from grains with nested items
fixup 4294d8b Support attributes from grains with nested items
fixup 4a7699b Support attributes from grains with nested items

  5. Save the file and exit the editor.

Git will apply all three changesets to a single commit, thereby cleaning up the history. The last step is to push the changes up to GitHub, but a normal push will fail since you have rewritten history that was previously pushed. Just run git push -f (force) and it should work fine.

Mapel88 commented 5 years ago

That's some crazy dark magic you have there... You assume I use git locally on my PC? I edited via github web editor, so my options are limited (at least I think so.. didn't find any rebase option here).

It might take me some time to fix as I'm pretty busy with work lately. Hope to get back to it soon.