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

Config compare#2948

Merged
dangogh merged 4 commits into
apache:masterfrom
ocket8888:config-compare
Oct 26, 2018
Merged

Config compare#2948
dangogh merged 4 commits into
apache:masterfrom
ocket8888:config-compare

Conversation

@ocket8888
Copy link
Copy Markdown
Contributor

What does this PR do?

Adds a script to generate configuration file routes for the compare tool. Also modifies the compare tool to be compatible with plaintext API endpoints (like most config files) and read input from stdin for easy piping between the two.

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?

First of all, be sure the documentation builds. Then, assuming the dependencies of the Traffic Ops Python client have been satisfied, try running the traffic_ops/testing/compare/genConfigRoutes.py script (see documentation for usage) and verify the output with the traffic_ops/testing/compare/compare.go tool (again, see documentation for usage)

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
Copy Markdown
Contributor

asfgit commented Oct 22, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2630/
Test PASSed.

@asfgit
Copy link
Copy Markdown
Contributor

asfgit commented Oct 23, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2632/
Test PASSed.

@asfgit
Copy link
Copy Markdown
Contributor

asfgit commented Oct 23, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2634/
Test PASSed.

Comment thread traffic_ops/testing/compare/compare.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We're not using the long option processing anywhere else.. Isn't the standard "flag" package sufficient?

If this is added, the source must be included in the vendor directory, license info properly documented in LICENSE and the appropriate lines added to .dependency_license and .rat-excludes. Is it worth doing that?

@ocket8888 ocket8888 force-pushed the config-compare branch 2 times, most recently from f2e619a to e9bb960 Compare October 23, 2018 18:39
@asfgit
Copy link
Copy Markdown
Contributor

asfgit commented Oct 23, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2637/
Test FAILed.

@asfgit
Copy link
Copy Markdown
Contributor

asfgit commented Oct 23, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2638/
Test PASSed.

@asfgit
Copy link
Copy Markdown
Contributor

asfgit commented Oct 23, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2639/
Test PASSed.

@dangogh dangogh self-assigned this Oct 23, 2018
@asfgit
Copy link
Copy Markdown
Contributor

asfgit commented Oct 24, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2643/
Test PASSed.

@asfgit
Copy link
Copy Markdown
Contributor

asfgit commented Oct 24, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2644/
Test PASSed.

@ocket8888 ocket8888 force-pushed the config-compare branch 2 times, most recently from 7289a18 to dbf9d32 Compare October 24, 2018 21:37
@asfgit
Copy link
Copy Markdown
Contributor

asfgit commented Oct 24, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2647/
Test PASSed.

@asfgit
Copy link
Copy Markdown
Contributor

asfgit commented Oct 24, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2648/
Test PASSed.

@ocket8888 ocket8888 force-pushed the config-compare branch 2 times, most recently from b1616c0 to b4a09bd Compare October 25, 2018 16:05
@asfgit
Copy link
Copy Markdown
Contributor

asfgit commented Oct 25, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2654/
Test FAILed.

@asfgit
Copy link
Copy Markdown
Contributor

asfgit commented Oct 25, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2655/
Test PASSed.

@mitchell852 mitchell852 added the Traffic Ops related to Traffic Ops label Oct 25, 2018
Also changed some of the logging to use more reasonable levels, without unnecessarily verbose stack traces for info messages
…le set of servers common between two Traffic Ops instances
@asfgit
Copy link
Copy Markdown
Contributor

asfgit commented Oct 26, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2664/
Test PASSed.

@asfgit
Copy link
Copy Markdown
Contributor

asfgit commented Oct 26, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2666/
Test PASSed.

@asfgit
Copy link
Copy Markdown
Contributor

asfgit commented Oct 26, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2667/
Test PASSed.

@dangogh dangogh merged commit 64e7ffa into apache:master Oct 26, 2018

# TOSession Class
class TOSession(restapi.RestApiSession):
"""
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Were all these whitespace changes really necessary? PEP8 recommends spaces over tabs https://www.python.org/dev/peps/pep-0008/#tabs-or-spaces.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I understand where you're coming from, but Tabs > spaces. Tabs are for indentation, spaces are for alignment. Also, consider from the first paragraph of PEP8:

This document gives coding conventions for the Python code comprising the standard library in the main Python distribution.

So it's not only a suggestion, but a suggestion for Python code intended for the standard library.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Regardless of whether or not tabs are better than spaces, following a set style guideline makes code easier to read when moving from project to project. In this case, spaces are preferred because they're consistent; a tab can be rendered differently in different views, whereas 4 spaces will always be rendered as 4 spaces. There are other PEP8 style guidelines that make a Python project read like Python, such as snake_case_function_names and CapWordsClassNames. It's like reading class my_java_class {...} in Java; it just doesn't read like Java.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1 on following PEP guidelines. I personally prefer tabs as well, but following the language idioms comes first.

In fact, I'd strongly support adopting a formatter (such as https://github.com/google/yapf) as a standard/requirement for the project, a la gofmt.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd love to see us using something like that as well, especially since we've been adding a lot of Python into the codebase lately. IMO gofmt was a huge win for Go.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have a Pylint configuration file I'll include next time I take a pass at the TO client (it needs to be a real package and not that weird symlink thing).
I disagree that spaces are consistent, tabs are consistent. A tab is one character, always, and always represents an indentation level. Your editor can display them at any desired width per your preferences, without sacrificing the actual contents of the files. That PEP is not meant to force a style guide on anyone using Python, it's specifically for contributors to the reference implementation's standard library.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You have to change your editor to choose a particular representation of the tab character, rather than it just being a consistent 4 spaces in any editor you use. For example, when writing the code you might have your editor set to 4 spaces for a tab. When viewing it in git diff, it might display as 8 spaces. In a GitHub PR it might be 6 spaces. It's just the style that's been chosen for Python for consistency and readability's sake.

Even though PEP8 mentions it is the style convention to be followed for the standard library, it's generally adopted by big open source Python projects (e.g. Django, OpenStack, Ansible). A major benefit to having a standard style of formatting is that you don't have to argue about the different ways things can be formatted; you just gofmt (or equivalent) the code and leave it at that. Generally for Python projects that is PEP8.

@ocket8888 ocket8888 deleted the config-compare branch November 19, 2018 14:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Traffic Ops related to Traffic Ops

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants