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

Added Travis CI, more checks to scripts and output more system information. #1435

Closed
wants to merge 2 commits into from

Conversation

tdulcet
Copy link
Contributor

@tdulcet tdulcet commented Sep 29, 2018

  • Added Travis CI (continuous integration). See the log here.
  • Added more checks to scripts.
    • Check that the user is on Linux before, checking that they are on Ubuntu 14.04/18.04.
    • Check internet connectivity, before preforming network checks, etc..
    • Output warning if hostname is not a valid FQDN.
    • Added temporary message to direct Ubuntu 18.04 users to the corresponding branch.
  • Outputs more system information (for user verification and debugging).
  • Outputs error messages to stderr.
  • Randomly generates a password for NONINTERACTIVE users that is more secure than 12345678.
  • Copy edited Readme and script documentation.
  • Fixed numerous bugs found by ShellCheck.

@JoshData
Copy link
Member

Hi. Thanks. There's a lot here.

In fact.... There's too much for me to review. Just as a practical matter, it'll take many months before I could finish going through all of the changes here.

I also strongly prefer to not mix logically unrelated changes in the same commit because it makes it very hard to review and in the future to use the history to pinpoint when things changed and for what reason. For instance, removing commas because of a personal preference about punctuation shouldn't be mixed with new features like setting up TravisCI. Each of your bullets in your summary above should be a separate commit (more or less).

Is there any chance you would submit a new pull request that makes the changes for just one of the logical things you were trying to do here?

@tdulcet
Copy link
Contributor Author

tdulcet commented Oct 14, 2018

@JoshData Thanks for looking at this.

Which one of the logical changes would you like me to make a pull request for? Or would you like me to make a new pull request for roughly each bullet? I considered doing that originally, but many of the bullets modify the same few files and I did not want to create a bunch of merge conflicts for you.

@@ -13,7 +15,7 @@ Our goals are to:

* Make deploying a good mail server easy.
* Promote [decentralization](http://redecentralize.org/), innovation, and privacy on the web.
* Have automated, auditable, and [idempotent](https://sharknet.us/2014/02/01/automated-configuration-management-challenges-with-idempotency/) configuration.
* Have automated, auditable and [idempotent](https://sharknet.us/2014/02/01/automated-configuration-management-challenges-with-idempotency/) configuration.
Copy link
Contributor

Choose a reason for hiding this comment

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

Research "Oxford Comma". As is, this change is likely to be rejected. ;)

@zatricky
Copy link
Contributor

zatricky commented Oct 15, 2018

There are many standards in use by many projects - but the main thing in common is the goal of making the changes easy to understand:

  • Pull requests should address a single topic
  • Commits should balance being as small and individually reviewable as possible against being overly burdensome on the persons committing or reviewing the code. Personally I err towards having better documentation for when someone is bugfixing and reviewing old changes.

@JoshData might prefer to do things differently of course, in particular with regard to the number of pull requests. His reply above implies perhaps he'd be okay with a single pull request - just not all in a single commit. The following is how I would likely structure my pull requests:

.travis.yml new file -> own commit -> travis-related pull request
Code of Conduct grammar -> own commit -> grammar-related PR
Contributing.md grammar -> own commit -> together with grammar-related PR
README.md grammar -> own commit -> together with grammar-related PR
README.md added URI/wiki links -> own commit -> own PR
daily_tasks.sh -> shared commit for linting -> linting-related PR
Security.md grammar -> own commit -> together with grammar-related PR
bootstrap.sh changes -> at least 3 commits -> "improved deploy"-related PR
dkim.sh linting -> shared commit for linting -> linting-related PR
dns.sh linting -> shared commit for linting -> linting-related PR
firstuser.sh spelling -> own commit -> together with grammar-related PR
firstuser.sh randomising PW -> own commit -> own PR
firstuser.sh linting -> shared commit for linting -> linting-related PR
functions.sh mktemp vs tempfile and removing the file -> own commit -> shared PR for functions.sh
functions.sh changes PACKAGES array -> own commit -> shared

And so forth

This might seem like a little more work for Josh to get through - but it's far easier. It gives your commits and pull requests focus. It also gives an opportunity for others to help spot and fix issues beforehand, which means there is less burden on Josh.

For your own peace of mind, it also means you can get some changes applied without depending on first resolving differences of opinion on unrelated changes, meaning the conversations don't lose focus.

@tdulcet
Copy link
Contributor Author

tdulcet commented Oct 25, 2018

@zatricky Thanks for your review and feedback.

@JoshData and @zatricky I divided this into three new pull requests: #1455, #1456, #1457.

I skipped all the spelling/grammar fixes in the markdown files, because I did not exhaustively fix all the mistakes, only the ones I noticed while making other changes and I did not feel there was enough to justify a separate pull request. Someone else is welcome to fix these errors.

@JoshData
Copy link
Member

Thanks. Closing this one then.

@JoshData JoshData closed this Oct 25, 2018
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