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

Refactoring code to refresh tokens #1325

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

soumyadipDe
Copy link
Contributor

Fixing #1323 while refactoring token management

@chandanchowdhury
Copy link
Collaborator

@soumyadipDe
Copy link
Contributor Author

We can, but for a simple fix it would too much for review IMO. Even the changes in this PR seems a bit too much, but still might be worth it for readability. Would the changes in PR looks too far from convention?

@achantavy
Copy link
Contributor

@soumyadipDe Thanks for the PR. I need a bit more time to review closely since it changes the structure of the sync.

It'd be amazing to add unit tests to ensure that things are stitched together properly, but I realize the original code didn't have it to begin with. Basically it'd be good to have some assurance that this works and some demonstration of the new behavior -- if unit tests are too much work, can you include either a log or screenshot showing that this works on your machine?

It will be a lot of work, but if you can use the new data model that will be great!

Nah, that is out of scope of this PR since the goal here is to fix a credential refresh problem.

@soumyadipDe
Copy link
Contributor Author

There's not much to show in logs as fix means there's no error. The end of it shows the time process had run for the whole process.

INFO:cartography.graph.job:Finished job gcp_dns_cleanup
INFO:cartography.graph.statement:Completed gcp_compute_asset_inet_exposure statement #1
INFO:cartography.graph.statement:Completed gcp_compute_asset_inet_exposure statement #2
INFO:cartography.graph.statement:Completed gcp_compute_asset_inet_exposure statement #3
INFO:cartography.graph.statement:Completed gcp_compute_asset_inet_exposure statement #4
INFO:cartography.graph.statement:Completed gcp_compute_asset_inet_exposure statement #5
INFO:cartography.graph.statement:Completed gcp_compute_asset_inet_exposure statement #6
INFO:cartography.graph.statement:Completed gcp_compute_asset_inet_exposure statement #7
INFO:cartography.graph.job:Finished job gcp_compute_asset_inet_exposure
INFO:cartography.graph.statement:Completed gcp_gke_asset_exposure statement #1
INFO:cartography.graph.statement:Completed gcp_gke_asset_exposure statement #2
INFO:cartography.graph.job:Finished job gcp_gke_asset_exposure
INFO:cartography.graph.statement:Completed gcp_gke_basic_auth statement #1
INFO:cartography.graph.statement:Completed gcp_gke_basic_auth statement #2
INFO:cartography.graph.job:Finished job gcp_gke_basic_auth
INFO:cartography.sync:Finishing sync stage 'gcp'
INFO:cartography.sync:Finishing sync with update tag '1720411083'

real    197m21.362s
user    6m20.901s
sys     0m15.675s

But this log was with this fix - https://github.com/lyft/cartography/pull/1324/files , not with this change with refactoring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants