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.

Sign up for GitHub

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

Jump to bottom

Onboard refactored tox & Travis CI setup and configuration vol. 1 #142

Merged
merged 1 commit into from
Jan 9, 2020
Merged

Onboard refactored tox & Travis CI setup and configuration vol. 1 #142

merged 1 commit into from
Jan 9, 2020

Conversation

i386x
Copy link
Contributor

@i386x i386x commented Jan 9, 2020

This is a first batch of changes introduced by PR #133 (changes are introduced in batches to make reviews easier). List of changes introduced in this PR:

  • document used/introduced environment variables in comments
  • use absolute pathes to config.sh and utils.sh
  • include config.sh unconditionally (config.sh is now a part of template, so it is created by autosynchronization script if it does not exist)
  • if python is a system python, install python3-selinux (python3-selinux is installed by default; in case there is a reason to not install python3-selinux, it can be removed from LSR_EXTRA_PACKAGES by config.sh)
  • install LSR_EXTRA_PACKAGES in one command (increase performance)

@coveralls
Copy link

coveralls commented Jan 9, 2020 •
edited
Loading

Pull Request Test Coverage Report for Build 420

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at ?%
Totals Coverage Status
Change from base Build 419: 0.0%
Covered Lines:
Relevant Lines: 0
💛 - Coveralls

tyll
tyll reviewed Jan 9, 2020
View reviewed changes
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.

Thank you, this is a lot nicer to review. I have just one small suggestion.

.travis/preinstall Outdated
# Add python3-selinux package (needed by Molecule on selinux enabled systems,
# because Molecule is using `copy` and `file` Ansible modules to setup the
# container).
if lsr_compare_pythons python -eq /usr/bin/python3; then
Copy link
Member

Choose a reason for hiding this comment

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

IMHO this comparison is hard to understand without knowing the previous context/digging deep into the code. How about adding a function venv_python_matches_system_python to give this a more descriptive name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

@i386x i386x force-pushed the lsr-template-refactored--part-1 branch from 2a9dd97 to 3a31f35 Compare January 9, 2020 15:02
@i386x
Copy link
Contributor Author

i386x commented Jan 9, 2020 •
edited
Loading

I accidentally squashed commits here instead of #143. Should I revert it back?

@tyll
Copy link
Member

tyll commented Jan 9, 2020

I accidentally squashed commits here instead of #143. Should I revert it back?

No, I do not need the fixup PRs.

@i386x i386x mentioned this pull request Jan 9, 2020
Merged
tyll
tyll approved these changes Jan 9, 2020
View reviewed changes
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.

Thank you, LGTM!.

@i386x @tyll
.travis/preinstall: Add python3-selinux
b563a09 
Add python3-selinux to the packages-to-be-installed list if
venv python matches system python.
@tyll tyll force-pushed the lsr-template-refactored--part-1 branch from 3a31f35 to b563a09 Compare January 9, 2020 18:51
@tyll tyll merged commit 7b5a06c into linux-system-roles:master Jan 9, 2020
@i386x
Copy link
Contributor Author

i386x commented Feb 10, 2020 •
edited
Loading

This PR reflects the changes in linux-system-roles/template@110a587. The requested changes based on review concerning .travis/utils.sh were squashed with linux-system-roles/template@c56d638.

@pcahyna
Copy link
Member

pcahyna commented Feb 10, 2020

@i386x why two commits ? Shouldn't be each PR for one commit? Otherwise how will you split synchronization when you synchronize template to another role?

@i386x
Copy link
Contributor Author

i386x commented Feb 11, 2020

@pcahyna I updated my previous comment to make it more clear: this PR was really based on one commit (linux-system-roles/template@110a587). However, those changes requested by reviewer related to .travis/utils.sh was squashed with linux-system-roles/template@c56d638.

@i386x i386x mentioned this pull request Feb 18, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tyll tyll tyll approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@i386x @coveralls @tyll @pcahyna