fix(install): replace `gid_from_name` (deprecated in `develop`) #40

ShaunMaher · 2020-07-16T08:27:43Z

PR progress checklist (to be filled in by reviewers)

Changes to documentation are appropriate (or tick if not required)
Changes to tests are appropriate (or tick if not required)
Reviews completed

What type of PR is this?

Primary type

[build] Changes related to the build system
[chore] Changes to the build process or auxiliary tools and libraries such as documentation generation
[ci] Changes to the continuous integration configuration
[feat] A new feature
[fix] A bug fix
[perf] A code change that improves performance
[refactor] A code change that neither fixes a bug nor adds a feature
[revert] A change used to revert a previous commit
[style] Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc.)

Secondary type

[docs] Documentation changes
[test] Adding missing or correcting existing tests

Does this PR introduce a `BREAKING CHANGE`?

No.

Related issues and/or pull requests

Close #39

Describe the changes you're proposing

Remove use of the deprecated "gid_from_name" property of user.present.

Pillar / config required to test the proposed changes

An existing working pillar should continue to work unmodified.

Debug log showing how the proposed changes work

I'm not sure how to do this, sorry.

Documentation checklist

Updated the README (e.g. Available states).
Updated pillar.example.

Testing checklist

Included in Kitchen (i.e. under state_top).
Covered by new/existing tests (e.g. InSpec, Serverspec, etc.).
Updated the relevant test pillar.

I'm not sure how to do this, sorry.

Additional context

pull-assistant · 2020-07-16T08:27:45Z

Best reviewed: commit by commit

Optimal code review plan

fix(install): replace gid_from_name (deprecated in develop)

Update install.sls

Update systemd.service.jinja

Update install.sls

Powered by Pull Assistant. Last update 127db73 ... 574775e. Read the comment docs.

ShaunMaher · 2020-07-17T08:51:42Z

Hi.

I don't think the build failures are because of anything I have changed but this is entirely new to me. Would anyone be able to look over the build log and, if it is me that has made an error, give me a hint?

Thanks
Shaun.

myii · 2020-07-17T17:08:18Z

etcd/install.sls

-  group.present:
-    - name: {{ etcd.group or 'etcd' }}
-    - system: True
-    - require_in:
-      - user: etcd-user-group-home


Why is this section being removed? I don't use this formula so I can't be sure if this should be done or not. If it should be, then it would be worth mentioning the rationale in the PR's main comment, so that future developers can understand what the thought process was.

Group is referenced here:

etcd-formula/etcd/install.sls

Line 40 in d4c849a

- group: {{ etcd.group or 'etcd' }}

myii · 2020-07-17T17:10:55Z

Thanks for preparing this PR, @ShaunMaher.

I don't think the build failures are because of anything I have changed but this is entirely new to me. Would anyone be able to look over the build log and, if it is me that has made an error, give me a hint?

Many of our formulas have well-maintained CI in place but no-one has got around to fixing things in this formula yet. Let's not worry about it too much right now. Once we're happy with the changes (see my inline comment), we won't let that get in the way of getting this merged.

noelmcloughlin · 2020-07-17T17:18:36Z

Hi @ShaunMaher I see myii is reviewing.

I just noticed a preexising bug

etcd-formula/etcd/files/systemd.service.jinja

Line 7 in d4c849a

User=root

Would you mind setting User=etcd in this file:
I think etcd should run non-privileged.
I checked and found a blog with User=etcd too: https://www.sma.im/install-etcd-as-systemd-service-on-ubuntu-server/

myii · 2020-07-17T17:22:53Z

@noelmcloughlin user is available in the map:

etcd-formula/etcd/defaults.yaml

Line 26 in d4c849a

user: etcd

Should that be repurposed to be used in the template?

noelmcloughlin · 2020-07-17T17:25:44Z

Should that be repurposed to be used in the template?

That makes sense - I have not looked at this formula in a long time - just noticed the PR in passing. Looks like CI needs work

noelmcloughlin · 2020-07-17T17:26:59Z

Should that be repurposed to be used in the template?

That makes sense - I have not looked at this formula in a long time - just noticed the PR in passing. Looks like CI needs work

Group is in the map too.

myii · 2020-07-17T17:30:07Z

etcd/install.sls

  user.present:
    - name: {{ etcd.user or 'etcd' }}
-    - gid_from_name: True
+    - gid: {{ etcd.user or 'etcd' }}


Based on @noelmcloughlin's comment, it looks like this should be:

Suggested change

- gid: {{ etcd.user or 'etcd' }}

- gid: {{ etcd.group or 'etcd' }}

noelmcloughlin · 2020-07-17T17:38:01Z

Except `gid` should be numeric - is gid needed - whatever gid the system allocates is fine.

…

On Fri, Jul 17, 2020 at 6:30 PM Imran Iqbal ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In etcd/install.sls <#40 (comment)> : > user.present: - name: {{ etcd.user or 'etcd' }} - - gid_from_name: True + - gid: {{ etcd.user or 'etcd' }} Based on @noelmcloughlin <https://github.com/noelmcloughlin>'s comment <https://github.com/saltstack-formulas/etcd-formula/pull/40/files#r456573876>, it looks like this should be: ⬇️ Suggested change - - gid: {{ etcd.user or 'etcd' }} + - gid: {{ etcd.group or 'etcd' }} — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#40 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADFUUQUZXZHLVZU4XGRGOWDR4CDC7ANCNFSM4O3Z4XDQ> .

ShaunMaher · 2020-07-20T07:46:29Z

Hi.

Sorry about the delay.

Thank you everyone for the feedback. I have made a couple of new changes, hopefully satisfying everyone.

@noelmcloughlin, according to the saltstack doco, gid doesn't have to be numeric: "The id of the default group to assign to the user. Either a group name or gid can be used. If not ..." (https://docs.saltstack.com/en/latest/ref/states/all/salt.states.user.html)

@myii, regarding "etcd.group" I agree, updated PR to reflect this change.

@myii, regarding the systemd unit template, I have included a fix. I have also wrapped User= and Group= lines in {%- if etcd.manage_users %} to match what is in install.sls.

Cheers.
Shaun.

myii · 2020-07-20T07:55:05Z

@ShaunMaher Thanks for making the changes. There's still this to be resolved either way: #40 (comment).

noelmcloughlin · 2020-07-20T08:10:49Z

Thanks @ShaunMaher

If we use group for gid then need to ensure group exists or was created before assigning gid:

ShaunMaher · 2020-07-20T08:59:11Z

Ok, yep. I was assuming that specifying a gid that didn't exist would have salt create the group. Never assume.

I'll restore "group.present" and the spots that included it as a require.

ShaunMaher · 2020-07-20T09:05:17Z

Ok. How about that?

myii

Thanks for working through this.

myii · 2020-07-20T09:06:40Z

@ShaunMaher Nice work, that's been merged.

noelmcloughlin · 2020-07-20T09:54:31Z

Thanks @ShaunMaher great cooperation and response!

fix(install): replace gid_from_name (deprecated in develop)

127db73

myii reviewed Jul 17, 2020

View reviewed changes

ShaunMaher added 2 commits July 20, 2020 17:35

Update install.sls

b52c646

Update systemd.service.jinja

092baff

Update install.sls

574775e

myii approved these changes Jul 20, 2020

View reviewed changes

myii merged commit 9603903 into saltstack-formulas:master Jul 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(install): replace `gid_from_name` (deprecated in `develop`) #40

fix(install): replace `gid_from_name` (deprecated in `develop`) #40

ShaunMaher commented Jul 16, 2020 •

edited by myii

Loading

pull-assistant bot commented Jul 16, 2020 •

edited

Loading

ShaunMaher commented Jul 17, 2020

myii Jul 17, 2020

noelmcloughlin Jul 17, 2020

myii commented Jul 17, 2020

noelmcloughlin commented Jul 17, 2020 •

edited

Loading

myii commented Jul 17, 2020

noelmcloughlin commented Jul 17, 2020

noelmcloughlin commented Jul 17, 2020

myii Jul 17, 2020

noelmcloughlin commented Jul 17, 2020 via email

ShaunMaher commented Jul 20, 2020

myii commented Jul 20, 2020

noelmcloughlin commented Jul 20, 2020

ShaunMaher commented Jul 20, 2020

ShaunMaher commented Jul 20, 2020

myii left a comment

myii commented Jul 20, 2020

noelmcloughlin commented Jul 20, 2020

	- gid: {{ etcd.user or 'etcd' }}
	- gid: {{ etcd.group or 'etcd' }}

fix(install): replace gid_from_name (deprecated in develop) #40

fix(install): replace gid_from_name (deprecated in develop) #40

Conversation

ShaunMaher commented Jul 16, 2020 • edited by myii Loading

PR progress checklist (to be filled in by reviewers)

What type of PR is this?

Primary type

Secondary type

Does this PR introduce a BREAKING CHANGE?

Related issues and/or pull requests

Describe the changes you're proposing

Pillar / config required to test the proposed changes

Debug log showing how the proposed changes work

Documentation checklist

Testing checklist

Additional context

pull-assistant bot commented Jul 16, 2020 • edited Loading

ShaunMaher commented Jul 17, 2020

myii Jul 17, 2020

Choose a reason for hiding this comment

noelmcloughlin Jul 17, 2020

Choose a reason for hiding this comment

myii commented Jul 17, 2020

noelmcloughlin commented Jul 17, 2020 • edited Loading

myii commented Jul 17, 2020

noelmcloughlin commented Jul 17, 2020

noelmcloughlin commented Jul 17, 2020

myii Jul 17, 2020

Choose a reason for hiding this comment

noelmcloughlin commented Jul 17, 2020 via email

ShaunMaher commented Jul 20, 2020

myii commented Jul 20, 2020

noelmcloughlin commented Jul 20, 2020

ShaunMaher commented Jul 20, 2020

ShaunMaher commented Jul 20, 2020

myii left a comment

Choose a reason for hiding this comment

myii commented Jul 20, 2020

noelmcloughlin commented Jul 20, 2020

fix(install): replace `gid_from_name` (deprecated in `develop`) #40

fix(install): replace `gid_from_name` (deprecated in `develop`) #40

ShaunMaher commented Jul 16, 2020 •

edited by myii

Loading

Does this PR introduce a `BREAKING CHANGE`?

pull-assistant bot commented Jul 16, 2020 •

edited

Loading

noelmcloughlin commented Jul 17, 2020 •

edited

Loading