Skip to content
This repository was archived by the owner on Nov 24, 2025. It is now read-only.

Add traffic_ops_ort.pl suuport for processing ##OVERRIDE## lines from…#3140

Merged
elsloo merged 1 commit intoapache:masterfrom
traeak:ort_anymap_override
Jan 9, 2019
Merged

Add traffic_ops_ort.pl suuport for processing ##OVERRIDE## lines from…#3140
elsloo merged 1 commit intoapache:masterfrom
traeak:ort_anymap_override

Conversation

@traeak
Copy link
Contributor

@traeak traeak commented Dec 18, 2018

fix to require space after ##OVERRIDE## keyword

What does this PR do?

Fixes #(issue_number)

Which TC components are affected by this PR?

  • Documentation
  • Grove
  • Traffic Analytics
  • Traffic Monitor
  • Traffic Ops
  • Traffic Ops ORT
  • Traffic Portal
  • Traffic Router
  • Traffic Stats
  • Traffic Vault
  • Other _________

What is the best way to verify this PR?

Select any existing base DS. Create ANY_MAP delivery service to override.
Create raw remap text with something like:

Short example:

##OVERRIDE## map http://from.foo http://to.bar

Make sure both the base DS and ANY_MAP are assigned to a server. run ORT in report mode to check that the ##OVERRIDE## rule is uncommented and that the base DS rule is commented out via ##OVERRIDDEN##.

Select to queue that particular server. Inspect the generated remap.config for the above conditions.
Select to queue again. Ensure that remap.config is not tagged as changed to avoid unnecessary config reloads.

Check all that apply

  • This PR includes tests
  • This PR includes documentation updates
  • This PR includes an update to CHANGELOG.md
  • This PR includes all required license headers
  • This PR includes a database migration (ensure that migration sequence is correct)
  • This PR fixes a serious security flaw. Read more: www.apache.org/security

@asfgit
Copy link
Contributor

asfgit commented Dec 18, 2018

Can one of the admins verify this patch?

Copy link
Contributor

@ocket8888 ocket8888 left a comment

Choose a reason for hiding this comment

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

Other than the one little thing this looks good to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

First let me say I'm a huge fan of turning space-based indentation into tab-based indentation. Just be careful that when you're doing that you aren't turning alignment into indentation. This should be indented one level with a tab character then aligned using spaces so that it displays consistently regardless of where your editor places its tabstops.

@traeak traeak force-pushed the ort_anymap_override branch from 5e5126b to a2f5fa4 Compare December 19, 2018 20:21
@traeak traeak force-pushed the ort_anymap_override branch from 6c41fbb to a29277c Compare January 7, 2019 23:26
Copy link
Contributor

@ocket8888 ocket8888 left a comment

Choose a reason for hiding this comment

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

In addition to my comments, I'd also suggest that ##OVERRIDE## always be wrapped double-backticks as it is literal text to be inserted in a configuration file.

For full documentation guidelines, refer to the documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please wrap console commands in double backticks like e.g.

(same as ``hostname -s``)

Copy link
Contributor

Choose a reason for hiding this comment

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

references to LSB commands (and/or those found on default installs of our only supported operating system - CentOS 7) should use the manpage role, e.g.

(same as :manpage:`hostname(1)`)

Copy link
Contributor

Choose a reason for hiding this comment

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

Syntax here is invalid; maintain the table structure by beginning/ending the frame with a pipe (|) as well as using it to delimit columns.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, please use "Delivery Service" rather than "deliveryservice".

Copy link
Contributor

@ocket8888 ocket8888 Jan 8, 2019

Choose a reason for hiding this comment

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

traffic_ops_ort.pl is a file, use the file role. Also acceptable is the program role. I also wouldn't request changes if it was simply wrapped in double-backticks, but file or program are better.

Example:

:file:`traffic_ops/bin/traffic_ops_ort.pl` or
:program:`traffic_ops_ort` (note no ``.pl``)

Copy link
Contributor

@ocket8888 ocket8888 Jan 8, 2019

Choose a reason for hiding this comment

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

Not only do you not need to do this, but it actually doesn't do anything. The index for the page can be generated simply by looking at the headings; no explicit index entries are necessary except to have the (sub)section title display differently in the sidebar than on the page (which you should NOT do)

Copy link
Contributor

@ocket8888 ocket8888 Jan 8, 2019

Choose a reason for hiding this comment

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

'ATS' is an abbreviation; if it's expanded previously in the section this could be left alone, but it isn't. The preferred way is to decorate it with the abbr role, but it would also be acceptable to place the expansion followed by the abbreviation in parenthesis.

Example:

:abbr:`ATS (Apache Traffic Server)` is the preferred abbreviation method
(especially since this isn't the abbreviation's first appearance in the section) but
"Apache Traffic Server (ATS)" could also be used.

I'd also argue that this is more of a warning than a note, but that's really up to your preference.

Copy link
Contributor

@ocket8888 ocket8888 Jan 8, 2019

Choose a reason for hiding this comment

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

don't use the double-colon. It'll try to highlight it using the default 'domain' of the page - which is 'Python' by default in Sphinx, but some pages set it to 'javascript' explicitly - this is neither. Instead, either wrap the text in double-backticks or use the code-block directive (with maybe the text syntax).

Example:

``this is preformatted text``

.. code-block:: text
    :caption: Example

    This is also preformatted, but is link-able and has a title/caption.
    If your text doesn't need to be linkable and/or shouldn't have a title/caption, then likely
    you don't need ``code-block`` and just want to use double-backticks.

Copy link
Contributor

Choose a reason for hiding this comment

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

again, no double colon

Copy link
Contributor

Choose a reason for hiding this comment

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

once more, no double colon

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a suggestion, this could link to an email thread/GitHub Issue/Pull Request. Or any combination thereof.

@traeak traeak force-pushed the ort_anymap_override branch 4 times, most recently from fe6c454 to 4f652fa Compare January 8, 2019 18:13
@mitchell852 mitchell852 added Traffic Ops ORT *DEPRECATED* related to the traffic_ops_ort.pl script new feature A new feature, capability or behavior labels Jan 8, 2019
Copy link
Contributor

Choose a reason for hiding this comment

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

The rest of the file (and the documentation) uses tabs for indentation. This table should too.

Copy link
Contributor

Choose a reason for hiding this comment

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

This table is malformed. You need to make sure your pipes align with the rest of the table.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is wrong, nevermind. I was looking at old changes :P

Copy link
Contributor

@ocket8888 ocket8888 Jan 9, 2019

Choose a reason for hiding this comment

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

These captions should be unique, and once again please use tabs not spaces.

@traeak traeak force-pushed the ort_anymap_override branch from 4f652fa to e3e4f9e Compare January 9, 2019 18:09
@elsloo elsloo self-assigned this Jan 9, 2019
@elsloo elsloo merged commit 443e5ba into apache:master Jan 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

new feature A new feature, capability or behavior Traffic Ops ORT *DEPRECATED* related to the traffic_ops_ort.pl script

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants