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

Servers RFC3339 timestamps#7718

Merged
zrhoffman merged 31 commits intoapache:masterfrom
ocket8888:to/servers-rfc3339
Aug 25, 2023
Merged

Servers RFC3339 timestamps#7718
zrhoffman merged 31 commits intoapache:masterfrom
ocket8888:to/servers-rfc3339

Conversation

@ocket8888
Copy link
Copy Markdown
Contributor

Related: #5911

This PR changes the lastUpdated field of server objects to use RFC3339 formatting to be consistent with all of the other timestamps used by that endpoint. It also makes some other tiny naming changes for consistency within the API: cachegroup and cachegroupId are now camelCase: cacheGroup and cacheGroupID. profileNames is now profiles becasue there is no other identifying information for any Profiles on server objects from which it needs to be distinguished. physLocation and physLocationId are now actual camelCase forms of "Physical Location" and "Physical Location ID", respectively. Finally, updPending and revalPending have been removed, because they provide no information that isn't trivially calculable from other properties, and are a remnant of a legacy system.

At a more implementation-detail-level, things which are never allowed to be nil according to API and/or database rules are now no longer allowed to be nil by Go's rules - because they aren't pointers anymore since they never needed to be. Also a lot of boilerplate from the method handlers was moved to a common API wrapper function.


Which Traffic Control components are affected by this PR?

  • Documentation
  • Traffic Ops

What is the best way to verify this PR?

Make sure the provided tests all pass.

PR submission checklist

  • This PR has tests
  • This PR has documentation
  • This PR has a CHANGELOG.md entry
  • This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY

@ocket8888 ocket8888 added Traffic Ops related to Traffic Ops documentation related to documentation low impact affects only a small portion of a CDN, and cannot itself break one tech debt rework due to choosing easy/limited solution labels Aug 15, 2023
@ocket8888 ocket8888 marked this pull request as ready for review August 15, 2023 16:14
@ocket8888 ocket8888 requested a review from srijeet0406 August 15, 2023 16:14
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 15, 2023

Codecov Report

Merging #7718 (0a9f4d4) into master (0c3a248) will increase coverage by 0.30%.
The diff coverage is 31.63%.

@@             Coverage Diff              @@
##             master    #7718      +/-   ##
============================================
+ Coverage     31.94%   32.24%   +0.30%     
============================================
  Files           710      681      -29     
  Lines         80707    79622    -1085     
  Branches        965      875      -90     
============================================
- Hits          25779    25677     -102     
+ Misses        52777    51833     -944     
+ Partials       2151     2112      -39     
Flag Coverage Δ
golib_unit 53.21% <61.32%> (+0.58%) ⬆️
grove_unit 12.02% <ø> (ø)
t3c_generate_unit ?
t3c_unit 5.99% <ø> (ø)
traffic_monitor_unit 26.33% <ø> (ø)
traffic_ops_integration 69.38% <64.28%> (-0.02%) ⬇️
traffic_ops_unit 22.00% <19.31%> (+0.11%) ⬆️
traffic_portal_v2 74.39% <ø> (ø)
traffic_router_unit ?
traffic_stats_unit 10.76% <ø> (ø)
unit_tests 29.39% <30.48%> (+0.18%) ⬆️
v3 57.79% <ø> (ø)
v4 79.18% <ø> (ø)
v5 78.49% <64.28%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
lib/go-tc/deliveryservice_servers.go 0.00% <0.00%> (ø)
lib/go-util/ptr.go 100.00% <ø> (ø)
...ffic_ops_golang/deliveryservice/servers/servers.go 30.88% <0.00%> (ø)
traffic_ops/traffic_ops_golang/server/servers.go 27.27% <10.96%> (-0.08%) ⬇️
...ops/traffic_ops_golang/deliveryservice/eligible.go 55.27% <50.00%> (ø)
traffic_ops/v5-client/server.go 67.32% <64.28%> (-0.64%) ⬇️
lib/go-tc/servers.go 40.87% <74.25%> (+12.27%) ⬆️
lib/go-util/collections.go 100.00% <100.00%> (ø)
traffic_ops/traffic_ops_golang/api/api.go 34.77% <100.00%> (+3.98%) ⬆️
...ffic_ops/traffic_ops_golang/api/shared_handlers.go 35.48% <100.00%> (+2.28%) ⬆️
... and 1 more

... and 31 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ocket8888 ocket8888 force-pushed the to/servers-rfc3339 branch 3 times, most recently from d97a48f to 1cb3a57 Compare August 21, 2023 15:29
@ocket8888
Copy link
Copy Markdown
Contributor Author

TP integration tests are failing because they always fail

@zrhoffman zrhoffman self-requested a review August 22, 2023 17:11
Comment thread .gitignore
Comment thread traffic_ops/v5-client/server.go
Comment thread traffic_ops/traffic_ops_golang/server/servers.go Outdated
Comment thread traffic_ops/traffic_ops_golang/server/servers.go Outdated
Comment thread traffic_ops/traffic_ops_golang/server/servers.go Outdated
Comment thread traffic_ops/traffic_ops_golang/server/servers.go Outdated
Comment thread traffic_ops/traffic_ops_golang/server/servers.go Outdated
@srijeet0406 srijeet0406 removed their request for review August 22, 2023 18:05
Copy link
Copy Markdown
Member

@zrhoffman zrhoffman left a comment

Choose a reason for hiding this comment

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

LessThan is logically the same as !GreaterThanOrEqualTo and LessThanOrEqualTo is logically the same is !GreaterThan, but sometimes one representation is more readable than another, and being constrained to have to use ! any time you want OrEqualTo makes the code less readable

You resolved this conversation but went ahead and did it anyway? Please revert the Version changes not directly related to this PR. If you still think those changes are worthwhile, you can open a separate PR with those changes.

@ocket8888
Copy link
Copy Markdown
Contributor Author

You resolved this conversation but went ahead and did it anyway? Please revert the Version changes not directly related to this PR. If you still think those changes are worthwhile, you can open a separate PR with those changes.

I... don't know what you mean? I resolved the conversation because I made changes which I believed resolved the conversation. You requested I do something, not that I not do anything?

@zrhoffman
Copy link
Copy Markdown
Member

You resolved this conversation but went ahead and did it anyway? Please revert the Version changes not directly related to this PR. If you still think those changes are worthwhile, you can open a separate PR with those changes.

I... don't know what you mean? I resolved the conversation because I made changes which I believed resolved the conversation. You requested I do something, not that I not do anything?

From the commit message of 324fe57

Also simplified those methods to just GreaterThan, LessThan and Equal, and removed unnecessary pointer recievers and arguments (all can just be pass-by-value).

That is what I was pushing back against in #7718 (comment) (and just now in #7718 (comment)). That part has no place in #7718, it messes with a bunch of stuff totally unrelated to using RFC 3339 timestamps for servers.

Comment thread traffic_ops/traffic_ops_golang/api/api.go Outdated
Comment thread traffic_ops/traffic_ops_golang/cdn/cdns.go Outdated
Comment thread traffic_ops/traffic_ops_golang/cdn/cdns.go Outdated
Comment thread traffic_ops/traffic_ops_golang/cdn_lock/cdn_lock.go Outdated
Comment thread traffic_ops/traffic_ops_golang/deliveryservice/eligible.go Outdated
Comment thread traffic_ops/traffic_ops_golang/server/servers.go Outdated
Comment thread traffic_ops/traffic_ops_golang/server/servers.go Outdated
Comment thread traffic_ops/traffic_ops_golang/server/servers.go Outdated
Comment thread traffic_ops/traffic_ops_golang/server/servers.go Outdated
Comment thread traffic_ops/traffic_ops_golang/server/servers.go Outdated
@ocket8888
Copy link
Copy Markdown
Contributor Author

You resolved this conversation but went ahead and did it anyway? Please revert the Version changes not directly related to this PR. If you still think those changes are worthwhile, you can open a separate PR with those changes.

I... don't know what you mean? I resolved the conversation because I made changes which I believed resolved the conversation. You requested I do something, not that I not do anything?

From the commit message of 324fe57

Also simplified those methods to just GreaterThan, LessThan and Equal, and removed unnecessary pointer recievers and arguments (all can just be pass-by-value).

That is what I was pushing back against in #7718 (comment) (and just now in #7718 (comment)).

Well, sure, but that was after I already made the changes and resolved the conversation. So I'm confused how you could think I "went ahead and did it anyway", since when I did it there couldn't have been any objections yet to that which hadn't been done.

That part has no place in #7718, it messes with a bunch of stuff totally unrelated to using RFC 3339 timestamps for servers.

Fair.

Copy link
Copy Markdown
Member

@zrhoffman zrhoffman left a comment

Choose a reason for hiding this comment

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

Looks good! Presumably the TPv2 tests due to the ChromeDriver version not being the very latest.

@zrhoffman zrhoffman merged commit 0fb433d into apache:master Aug 25, 2023
@ocket8888 ocket8888 deleted the to/servers-rfc3339 branch August 25, 2023 19:08
rimashah25 added a commit to rimashah25/trafficcontrol that referenced this pull request Dec 4, 2023
…pache#7718 (apache#49)

* Updated TP field names based on TO changes from PRs apache#7806, apache#7718,

* Updated TP field name (cdn) in server capability and updated changelog
rimashah25 added a commit to rimashah25/trafficcontrol that referenced this pull request Dec 4, 2023
…pache#7718

Updated TP field name (cdn) in server capability and updated changelog
rimashah25 added a commit to rimashah25/trafficcontrol that referenced this pull request Dec 4, 2023
…pache#7718

Updated TP field name (cdn) in server capability and updated changelog
zrhoffman pushed a commit that referenced this pull request Dec 4, 2023
* * Fixed broken capability links for DS

* Updated CHANGELOG.md

* Updated based on review comment as well as removed deleteServerCapability button(DS table) and menu-option(right click)

* Updated TP field names based on TO changes from ATC PRs #7806, #7718

Updated TP field name (cdn) in server capability and updated changelog

* Updated broken links in DS's right click menu
rimashah25 added a commit that referenced this pull request Jan 3, 2024
Updated based on review comment as well as removed deleteServerCapability button(DS table) and menu-option(right click)
Updated TP field names based on TO changes from ATC PRs #7806, #7718
Updated TP field name (cdn) in server capability and updated changelog
Updated broken links in DS's right click menu
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

documentation related to documentation low impact affects only a small portion of a CDN, and cannot itself break one tech debt rework due to choosing easy/limited solution Traffic Ops related to Traffic Ops

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants