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

Update docker hacking information #744

Merged
merged 3 commits into from
Nov 1, 2019
Merged

Update docker hacking information #744

merged 3 commits into from
Nov 1, 2019

Conversation

luke-hill
Copy link
Contributor

Take 3 :) This is a follow-on from #649

NB: There are other links to travis in the HACKING.md file, but I don't know enough about this to fix it. So this is still needing to be updated. I've made an issue item about this to be tidied up

Summary

Update a few review points leftover that were added by aslak after previous PR was merged

Types of changes

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist:

  • The change has been ported to Java.
  • The change has been ported to Ruby.
  • The change has been ported to JavaScript.
  • The change has been ported to Go.
  • The change has been ported to .NET.
  • I've added tests for my code.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@luke-hill
Update docker hacking information, replace a couple of vague referenc…
0461cd0 
…es to Travis

NB: There is other links to travis in the HACKING.md file, but I don't know enough about this to fix it. So this is still needing to be updated
mlvandijk
mlvandijk requested changes Oct 10, 2019
View reviewed changes
Copy link
Member

@mlvandijk mlvandijk left a comment

Choose a reason for hiding this comment

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

Small comments. Still have not tried this out to see if anything is missing...

HACKING.md Outdated

To start with, we need to ensure we have Docker installed. This is usually fairly easy, and a comprehensive set
of setup instructions are available [HERE](https://docs.docker.com/install/) for most OS'
To start with, we need to ensure we have Docker installed. This is fairly straightforward, and a comprehensive set
Copy link
Member

Choose a reason for hiding this comment

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

Let's not use words like "easy", "simple", "straightforward" in any of the documentation. (see also: cucumber/docs#389)

HACKING.md Show resolved Hide resolved
@aslakhellesoy aslakhellesoy added 📖 documentation Improvements or additions to documentation 🔧 build Related to build / release process labels Oct 14, 2019
@mlvandijk mlvandijk mentioned this pull request Oct 25, 2019
Open
@luke-hill
Copy link
Contributor Author

Is this good to be merged? It's a slight improvement again...

@aslakhellesoy / @mlvandijk

@luke-hill
Copy link
Contributor Author

I'm gonna merge this in. We can tidy things up later if needs be again. It's a slight improvement.

@luke-hill luke-hill merged commit 719490b into master Nov 1, 2019
@luke-hill luke-hill deleted the feature/update_docker_hacking_info branch November 1, 2019 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mlvandijk mlvandijk mlvandijk requested changes

@aslakhellesoy aslakhellesoy Awaiting requested review from aslakhellesoy

Assignees
No one assigned
Labels
📖 documentation Improvements or additions to documentation 🔧 build Related to build / release process
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@luke-hill @mlvandijk @aslakhellesoy