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

Fixed ASN Update logic for APIv5 #7767

Merged
merged 2 commits into from
Aug 31, 2023

Conversation

rimashah25
Copy link
Contributor

@rimashah25 rimashah25 commented Aug 31, 2023

This PR fixes a bug in the ASN update logic in TO APIv5 where changing the ASN of an ASN is erroneously not allowed.
Related: #7561


Which Traffic Control components are affected by this PR?

  • Traffic Ops

What is the best way to verify this PR?

Test PUT API call (v5) for the following cases:

  1. Update existing ASN to another existing ASN (result: fail, since ASN exists)
  2. Update existing ASN to a new non-existent ASN (result: pass)
  3. Update existing ASN's cachegroup (result: pass)
  4. Update existing ASN with a new ASN and a different cachegroup (result: pass)

If this is a bugfix, which Traffic Control versions contained the bug?

master

PR submission checklist

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

@rimashah25 rimashah25 added Traffic Ops related to Traffic Ops regression bug a bug in existing functionality introduced by a new version medium impact impacts a significant portion of a CDN, or has the potential to do so low difficulty the estimated level of effort to resolve this issue is low labels Aug 31, 2023
@rimashah25 rimashah25 marked this pull request as ready for review August 31, 2023 16:39
@codecov
Copy link

codecov bot commented Aug 31, 2023

Codecov Report

Merging #7767 (ea60754) into master (e835435) will increase coverage by 4.02%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master    #7767      +/-   ##
============================================
+ Coverage     27.87%   31.90%   +4.02%     
  Complexity       98       98              
============================================
  Files           538      712     +174     
  Lines         74980    81914    +6934     
  Branches         90      965     +875     
============================================
+ Hits          20902    26136    +5234     
- Misses        52080    53633    +1553     
- Partials       1998     2145     +147     
Flag Coverage Δ
golib_unit 53.70% <ø> (ø)
grove_unit 12.02% <ø> (ø)
t3c_unit 5.99% <ø> (ø)
traffic_monitor_unit 26.33% <ø> (ø)
traffic_ops_integration 69.38% <ø> (+3.74%) ⬆️
traffic_ops_unit 21.64% <0.00%> (ø)
traffic_portal_v2 74.39% <ø> (?)
traffic_stats_unit 10.78% <ø> (ø)
unit_tests 29.21% <0.00%> (+3.48%) ⬆️
v3 57.79% <ø> (ø)
v4 79.18% <ø> (?)
v5 78.49% <ø> (ø)

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

Files Changed Coverage Δ
traffic_ops/traffic_ops_golang/asn/asns.go 14.15% <0.00%> (ø)

... and 174 files with indirect coverage changes

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

@shamrickus shamrickus self-assigned this Aug 31, 2023
Copy link
Member

@shamrickus shamrickus left a comment

Choose a reason for hiding this comment

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

LGTM

  • ASNs endpoint appears to work through the api and fully works through TP

@shamrickus shamrickus merged commit 2257ddb into apache:master Aug 31, 2023
82 of 83 checks passed
@rimashah25 rimashah25 deleted the bugfix/asn-update branch August 31, 2023 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
low difficulty the estimated level of effort to resolve this issue is low medium impact impacts a significant portion of a CDN, or has the potential to do so regression bug a bug in existing functionality introduced by a new version Traffic Ops related to Traffic Ops
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants