Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add empty timesync_ntp_provider to defaults #15

Merged
merged 1 commit into from May 25, 2019

Conversation

mlichvar
Copy link
Collaborator

Set the variable in defaults/main.yml to an empty string, so all input variables of the role are included there.

This PR includes #14 to avoid merge conflicts.

@pcahyna
Copy link
Member

pcahyna commented Jul 24, 2018

Is it really necessary for anything? I would prefer to leave the variable undefined if not set by the user.

@mlichvar
Copy link
Collaborator Author

It's not really necessary, but I think it's good for consistency. All input variables are defined and they are all listed in the same file.

@tyll
Copy link
Member

tyll commented Jul 26, 2018

[citest pending]

@tyll
Copy link
Member

tyll commented Jul 27, 2018

[citest bad]

1 similar comment
@tyll
Copy link
Member

tyll commented Jul 27, 2018

[citest bad]

@tyll
Copy link
Member

tyll commented Oct 10, 2018

One might expect the defaults file to specify the API of a role, since it contains default values for the variables that are expected to be overridden by a role. Therefore it might be a good idea to specify the variable there.

@tyll
Copy link
Member

tyll commented May 23, 2019

This is now needed for Satellite - @mlichvar there is now a conflict, can you please resolve it? Maybe wait a little since I might be merging other PRs as well.

@tyll tyll self-requested a review May 25, 2019 10:09
Copy link
Member

@tyll tyll left a comment

Choose a reason for hiding this comment

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

LGTM, merged the conflict.

@tyll tyll merged commit 00a646c into linux-system-roles:master May 25, 2019
@pcahyna
Copy link
Member

pcahyna commented May 27, 2019

Indeed, this helps Foreman/Satellite, cf: linux-system-roles/selinux#43

I would like to settle on a common convention, employed in that PR: the special value used to indicate that a setting should be preserved should be null (instead of an empty string). Would that be ok?

@tyll
Copy link
Member

tyll commented May 27, 2019

Indeed, this helps Foreman/Satellite, cf: linux-system-roles/selinux#43

I would like to settle on a common convention, employed in that PR: the special value used to indicate that a setting should be preserved should be null (instead of an empty string). Would that be ok?

This sounds ok on first thought. Please note, that his PR is already merged, so we need a new one to do this. Also we should document this decision in a design decisions or best practices document and have the discussion there. This makes it easier to discover than a PR for just one role.

IMHO the document could be added to https://github.com/linux-system-roles/linux-system-roles.github.io

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants