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

Removing restangular - #3595

Merged
mitchell852 merged 8 commits into
apache:masterfrom
ocket8888:removing-restangular
Dec 17, 2019
Merged

mitchell852 merged 8 commits into
apache:masterfrom
ocket8888:removing-restangular

Conversation

@ocket8888

@ocket8888 ocket8888 commented May 22, 2019 •

Copy link
Copy Markdown
Contributor

What does this PR (Pull Request) do?

Removes a dependency that Traffic Portal had on Restangular.

Which Traffic Control components are affected by this PR?

  • Traffic Portal

Traffic Portal dependencies are not individually documented, and so no changes are necessary.

What is the best way to verify this PR?

No functionality should have changed (except that errors will now be logged instead of ignored in many cases), so the existing tests should all pass.

Jeremy: actually, the only way to truly verify that the functionality did not change is to:

  1. run the UI tests (which is a very small subset of all TP functionality)
  2. manually verify the functionality associated with ALL the services functions that changed just to be sure all is well.

The following criteria are ALL met by this PR

  • This PR includes tests OR I have explained why tests are unnecessary
  • I have explained why documentation is unnecessary
  • This PR includes an update to CHANGELOG.md
  • This PR includes any and all required license headers
  • This PR does not include a database migration
  • This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY

@mitchell852 mitchell852 added Traffic Portal v1 related to Traffic Portal version 1 improvement The functionality exists but it could be improved in some way. labels May 22, 2019
@ocket8888
ocket8888 force-pushed the removing-restangular branch from 4631e74 to 855943f Compare May 22, 2019 20:05
@asfgit

asfgit commented May 22, 2019

Copy link
Copy Markdown
Contributor

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

Comment thread traffic_portal/app/src/common/api/ASNService.js Outdated

@mitchell852 mitchell852 left a comment

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.

still reviewing but here's what i've found so far

Comment thread traffic_portal/app/src/common/api/AuthService.js Outdated
Comment thread traffic_portal/app/src/common/api/CDNService.js Outdated
Comment thread traffic_portal/app/src/common/api/CDNService.js Outdated
Comment thread traffic_portal/app/src/common/api/CapabilityService.js Outdated
Comment thread traffic_portal/app/src/common/api/CoordinateService.js Outdated
@mitchell852 mitchell852 added tech debt rework due to choosing easy/limited solution and removed improvement The functionality exists but it could be improved in some way. labels May 22, 2019
@asfgit

asfgit commented May 23, 2019

Copy link
Copy Markdown
Contributor

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

@mitchell852 mitchell852 left a comment

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.

lots of stuff broken. better retest.

Comment thread traffic_portal/app/src/common/api/ChangeLogService.js Outdated
Comment thread traffic_portal/app/src/common/api/AuthService.js Outdated
Comment thread traffic_portal/app/src/common/api/DeliveryServiceService.js Outdated
Comment thread traffic_portal/app/src/common/api/DeliveryServiceService.js Outdated
Comment thread traffic_portal/app/src/common/api/DeliveryServiceSslKeysService.js Outdated
Comment thread traffic_portal/app/src/common/api/DeliveryServiceUriSigningKeysService.js Outdated
@ocket8888
ocket8888 force-pushed the removing-restangular branch from 36e9ad8 to 34bad7c Compare May 24, 2019 14:55
@asfgit

asfgit commented May 24, 2019

Copy link
Copy Markdown
Contributor

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

Comment thread traffic_portal/app/src/common/api/CDNService.js Outdated
Comment thread traffic_portal/app/src/common/api/UserService.js Outdated
Comment thread traffic_portal/app/src/common/api/UserService.js Outdated
@asfgit

asfgit commented May 28, 2019

Copy link
Copy Markdown
Contributor

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

@asfgit

asfgit commented May 28, 2019

Copy link
Copy Markdown
Contributor

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

Comment thread traffic_portal/app/src/app.js 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.

this whole timeout thing is weird. maybe it can be removed. i'll test it out. can't remember why i put it in here.

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.

When I remove it it also doesn't show up messages

@mitchell852

Copy link
Copy Markdown
Member

the more I think about it, I think this PR is just way too big / risky. it requires a lot of positive and negative testing to ensure it does not change/break functionality. imo you should tackle one "service" at a time. i.e. remove the use of restangular from cdnservice.js. and move down the list of "services" with a seperate PR until nothing uses restangular and the dependency can be removed altogether. otherwise, the risk is much too high given that TP doesn't have comprehensive regression tests in place.

@ocket8888

Copy link
Copy Markdown
Contributor Author

Alright man, you asked for it

I'm gonna leave this open as a draft - I'll rebase once the others are all merged and this will be the one that actually removes restangular entirely.

@ocket8888 ocket8888 changed the title Removing restangular WIP - Removing restangular May 28, 2019
@mitchell852 mitchell852 added the WIP "Work-in-Progress" - do not merge! (use 'draft' pull requests from now on) label May 31, 2019
@ocket8888
ocket8888 force-pushed the removing-restangular branch from 72420af to e1ea540 Compare November 7, 2019 18:51
@asf-ci

asf-ci commented Nov 7, 2019

Copy link
Copy Markdown
Contributor

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

@mitchell852

Copy link
Copy Markdown
Member

@ocket8888 to rebase

@ocket8888
ocket8888 force-pushed the removing-restangular branch from e1ea540 to f9b1599 Compare December 16, 2019 17:42
@ocket8888 ocket8888 removed the WIP "Work-in-Progress" - do not merge! (use 'draft' pull requests from now on) label Dec 16, 2019
@asf-ci

asf-ci commented Dec 16, 2019

Copy link
Copy Markdown
Contributor

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

@ocket8888 ocket8888 changed the title WIP - Removing restangular Removing restangular Dec 16, 2019
@asf-ci

asf-ci commented Dec 16, 2019

Copy link
Copy Markdown
Contributor

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

@mitchell852 mitchell852 left a comment

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.

make sure to remove from traffic_portal/grunt/browserify2.js as well

@ocket8888
ocket8888 force-pushed the removing-restangular branch from 985fce7 to 8109386 Compare December 16, 2019 21:36
@asf-ci

asf-ci commented Dec 16, 2019

Copy link
Copy Markdown
Contributor

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

@asf-ci

asf-ci commented Dec 16, 2019

Copy link
Copy Markdown
Contributor

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

Comment thread traffic_portal/app/src/common/api/ProfileParameterService.js Outdated
@rawlinp rawlinp added this to the 4.0.0 milestone Dec 17, 2019
@mitchell852
mitchell852 dismissed their stale review December 17, 2019 16:46

changes requested were made

@mitchell852
mitchell852 merged commit aff0186 into apache:master Dec 17, 2019
@ocket8888
ocket8888 deleted the removing-restangular branch December 17, 2019 17:00
@asf-ci

asf-ci commented Dec 17, 2019

Copy link
Copy Markdown
Contributor

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

tech debt rework due to choosing easy/limited solution Traffic Portal v1 related to Traffic Portal version 1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TP: Generate ISO view won't resolve TP: remove Restangular dependency

5 participants