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

Minor updates to mariadb/README.md #2055

Merged
merged 1 commit into from
Nov 5, 2021
Merged

Conversation

LeamHall
Copy link
Contributor

Changed ":tag" to ":latest".
Added user to --env option usage.
Added some notes on cleanup.

@LeamHall
Copy link
Contributor Author

Updated mariadb/content.md, once DBlack pointed me to the right file to edit.

@tianon
Copy link
Member

tianon commented Oct 22, 2021

Still a few failing CI bits -- would it be helpful for us to take over and get this reformatted to bring it across the finish line? 😅

Comment on lines 84 to 92
```console
$ docker ps
```

To get the IP address of your server instance, use the container ID given above:

```console
$ docker inspect <server container ID> |grep IPAddress
```
Copy link
Member

Choose a reason for hiding this comment

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

We try not to use our somewhat limited space on the Hub docs to document basic Docker usage, but it's really up to each image maintainer what info they want in here -- cc @grooverdan @dbart 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I hadn't thought of that! I am very new to this and look to you all for critique. To answer the question about failing CI bits, I'm fine with a smarter person taking over. I would like to see what I got wrong, so I can learn. Would it be easier to give you access to my repo, or is there a better way?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I've got direct commit bits on this branch already thanks to you leaving the "Allow edits and access to secrets by maintainers" checkbox enabled.

The CI failures are pretty straightforward, but a little tedious (which is why I offered to take care of them).

The markdownfmt failure is because we use https://github.com/tianon/markdownfmt to make sure our markdown is consistently formatted (which makes future PRs easier to deal with since there won't be as many unrelated formatting changes in them). There's a markdownfmt.sh script in this repository that can help with invoking the tool locally via Docker -- the UX of it is similar to gofmt if you've ever used that; -d gives a diff (which is what the CI failure's logs include) and -w will auto-write the formatted files.

The other failure is because you've now updated content.md appropriately, but still have the changes to README.md in the PR. The best fix is to do something like an interactive rebase to remove them, although the usage of Git to do so can get a little arcane.

Regarding updating this content, I'm going to defer that choice to @grooverdan and @dbart since it's their image (but I'm happy to help make any content changes too if you/they want them). 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use git daily, but not at the arcane level. At this point I think we're all safer if you make the changes. For the record, I got started on this while learning to build a Docker MariaDB instance so I could re-learn Go and play with a database. go fmt is an unruly friend. :)

@grooverdan, I agree with @tianon's comment about removing basic Docker info from the file.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy for the generic bits of docker networking to be minimized/eliminated.

I've seen many uses over literal "tag" being used so latest is ok by me.

The small bit below on doing a connection test is fine. With --rm specified on the command line, stop & rm of the client container isn't needed. Thanks @LeamHall and @tianon

Copy link
Member

Choose a reason for hiding this comment

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

@LeamHall want to make the content changes and then I'll worry about the formatting/commit history changes? I'm happy to help as much or as little as you'd like here. 😄

Copy link
Contributor Author

@LeamHall LeamHall Oct 27, 2021

Choose a reason for hiding this comment

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

@tianon, my apologies for taking so long to get back to you. It looks like I forgot to tag lines 68-73 with ```console. Is there a tool that I can run to check the document before a commit?

Addendum Found the markdown checker. I'll go over the content notes and markdown stuff today, and let you know what I could get done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just pushed the changes, can you give it a look?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good. To keep the CI tools happy README.md needs to be left out of the changes.

@tianon
Copy link
Member

tianon commented Nov 5, 2021

Just did the rebase/force push dance:

$ git pull --rebase https://github.com/docker-library/docs.git master # rebase to the latest upstream commit
$ git rebase -i FETCH_HEAD # squash all the commits down to just one
$ git show FETCH_HEAD:mariadb/README.md > mariadb/README.md # revert README.md changes
$ git add mariadb/README.md # stage the change
$ git commit --amend # add the change to the commit
$ git push -f # force push back to the branch to update the PR

Thanks for working through this!

@tianon tianon merged commit eece3f9 into docker-library:master Nov 5, 2021
@LeamHall
Copy link
Contributor Author

LeamHall commented Nov 6, 2021

Guys, thank you for your expertise!

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.

3 participants