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

docs(serializers): fix hyperlinks to salt.serializers #56325

Merged
merged 13 commits into from
Apr 16, 2020

Conversation

myii
Copy link
Contributor

@myii myii commented Mar 8, 2020

What does this PR do?

While reviewing saltstack-formulas/telegraf-formula#4, found that the link to salt.serializers (serializer modules) wasn't working at:

Found a couple of other places in the docs this wasn't working, either:

Tested the fix by building the docs locally. Also found a couple of further documentation issues which were resolved in the other two commits.

What issues does this PR fix or reference?

N/a.

Tests written?

N/a, docs only.

Commits signed with GPG?

Yes.

@myii myii requested a review from a team as a code owner March 8, 2020 09:13
@ghost ghost requested a review from waynew March 8, 2020 09:13
@waynew waynew added Bug broken, incorrect, or confusing behavior Documentation Relates to Salt documentation labels Mar 10, 2020
@waynew
Copy link
Contributor

waynew commented Mar 10, 2020

@myii Thanks for the fix! Would you also go ahead and add this to the fixed section in the CHANGELOG.md?

@myii
Copy link
Contributor Author

myii commented Mar 11, 2020

... Would you also go ahead and add this to the fixed section in the CHANGELOG.md?

@waynew Would that be under 3000.1? Also, couldn't this end up leading to various merge conflicts across PRs as each change the same lines in the changelog?

@waynew
Copy link
Contributor

waynew commented Mar 11, 2020

@myii it shouldn't - we've added it to the .gitattributes file that should just merge it all. If that doesn't work we'll have to take a different approach. But also we've been lazy about asking people to update the changelog 😝

@myii
Copy link
Contributor Author

myii commented Mar 11, 2020

@waynew OK, done. I'll do this for #56237 as well.

```
./salt/doc/topics/development/contributing.rst:142: WARNING: Error in "code-block" directive:
1 argument(s) required, 0 supplied.

.. code-block::

    Fix broken things in file1 and file2

    Fixes saltstack#31337

    We needed to make this change because the underlying dependency
    changed. Now this uses the up-to-date API.

    # Please enter the commit message for your changes. Lines starting
    # with '#' will be ignored, and an empty message aborts the commit.
    # On branch fix-broken-thing
    # Changes to be committed:
    #       modified:   path/to/file1
    #       modified:   path/to/file2
./salt/doc/topics/development/contributing.rst:160: WARNING: Error in "code-block" directive:
1 argument(s) required, 0 supplied.

.. code-block::

    Fixes broken things

    # Please enter the commit message for your changes. Lines starting
    # with '#' will be ignored, and an empty message aborts the commit.
    # On branch fix-broken-thing
    # Changes to be committed:
    #       modified:   path/to/file1
    #       modified:   path/to/file2
./salt/doc/topics/releases/3000.rst:558: WARNING: Error in "code-block" directive:
1 argument(s) required, 0 supplied.

.. code-block::

  <source>
    @type forward
    port 24224
  </source>
  <match saltstack.**>
    @type file
    path /var/log/td-agent/saltstack
  </match>
```
@myii myii force-pushed the docs/fix-links-to-serializers branch from b1d2ea0 to ec4e332 Compare March 11, 2020 15:01
CHANGELOG.md Outdated
@@ -166,6 +166,7 @@ Versions are `MAJOR.PATCH`.
- [#55823](https://github.com/saltstack/salt/pull/55823) - Fix issue with overly long names in the LGPO module - [@twangboy](https://github.com/twangboy)
- [#55843](https://github.com/saltstack/salt/pull/55843) - Fixed `file.mkdir` to respect `test=True` - [@mchugh19](https://github.com/mchugh19)
- [#55845](https://github.com/saltstack/salt/pull/55845) - Fixed logging to return multiprocessing queue if it's already set - [@s0undt3ch](https://github.com/s0undt3ch)
- [#56325](https://github.com/saltstack/salt/pull/56325) - Fix hyperlinks to `salt.serializers` and other documentation issues - [@myii](https://github.com/myii)
Copy link
Contributor

Choose a reason for hiding this comment

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

Oooh, I think you need to rebase - the version above is ## Unreleased (Neon). The latest master should have 3000.1, though given our current schedule this will probably actually end out in ## Unreleased - 3001 (Magnesium), which I don't think I added yet 🙃

Copy link
Contributor

Choose a reason for hiding this comment

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

e3216db That comment was correct - I hadn't added the Magnesium release to the top of the changelog

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@waynew Done, apologies for the mix-up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@waynew I just noticed:

### Unreleased (3000.1)

Surely that should be:

-### Unreleased (3000.1)
+## Unreleased (3000.1)

Copy link
Contributor

Choose a reason for hiding this comment

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

🤦‍♂ doh! My bad. Yes it should. We'll probably fix that with Sodium - the changelog is important, but I wouldn't say it's critical 😝

waynew
waynew previously approved these changes Mar 11, 2020
@frogunder
Copy link
Contributor

@myii

Seems like there is a conflict, can you take a look, please?

@myii
Copy link
Contributor Author

myii commented Mar 16, 2020

@frogunder #56325 (comment).

@myii it shouldn't - we've added it to the .gitattributes file that should just merge it all. If that doesn't work we'll have to take a different approach. But also we've been lazy about asking people to update the changelog stuck_out_tongue_closed_eyes

@waynew Conflicting files: CHANGELOG.md! How would you like me to deal with this?

@waynew
Copy link
Contributor

waynew commented Mar 16, 2020

@myii I went ahead and resolved it. And fixed the ### 3000.1 -> ## 3000.1 thing 👍

@myii
Copy link
Contributor Author

myii commented Mar 16, 2020

@myii I went ahead and resolved it. And fixed the ### 3000.1 -> ## 3000.1 thing 👍

Thanks!

@frogunder frogunder added Merge Ready ZRelease-Sodium retired label and removed Bug broken, incorrect, or confusing behavior Documentation Relates to Salt documentation labels Mar 17, 2020
@frogunder frogunder removed the ZRelease-Sodium retired label label Mar 19, 2020
CHANGELOG.md Outdated
@@ -6,7 +6,7 @@ This changelog follows [keepachangelog](https://keepachangelog.com/en/1.0.0/) fo
This project versioning is _similar_ to [Semantic Versioning](https://semver.org), and is documented in [SEP 14](https://github.com/saltstack/salt-enhancement-proposals/pull/20/files).
Versions are `MAJOR.PATCH`.

### 3000.1
## 3000.1
Copy link
Contributor

@Ch3LL Ch3LL Apr 15, 2020

Choose a reason for hiding this comment

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

We already released 3000.1 so this would make it into Sodium (3001)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ch3LL Done, I had to prepare the section for 3001 as well. I took the liberty to move my other entry to 3001 (#56237), since that was merged after the release of 3000.1 as well.

@Ch3LL Ch3LL removed the Merge Ready label Apr 15, 2020
@Ch3LL
Copy link
Contributor

Ch3LL commented Apr 16, 2020

@waynew are you good with this? Just waiting on your approval here

@waynew
Copy link
Contributor

waynew commented Apr 16, 2020

@Ch3LL 🚀

@Ch3LL Ch3LL merged commit 51fd70f into saltstack:master Apr 16, 2020
@myii myii deleted the docs/fix-links-to-serializers branch April 16, 2020 14:38
@sagetherage sagetherage added the ZRelease-Sodium retired label label May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ZRelease-Sodium retired label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants