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

[CDF-24184] 🚍 Strongly coupled views. #1490

Merged
merged 8 commits into from
Mar 3, 2025
Merged

Conversation

doctrino
Copy link
Collaborator

@doctrino doctrino commented Mar 1, 2025

Description

Data models with strongly coupled views through implements and reverse direct relations are tricky to deploy as the API returns 400 complaining about views missing which are part of the request.

image

Isolating exactly what causes this false 400 + does not exists turned out to be extremely hard, it seem that you need a model of a certain size to trigger this behavior. Ended up using a larger model with lots of coupling to consistently trigger the error.

Node The relevant team has been notified, but as we have users waiting for this, I suggest we do merge this PR as soon as possible to have it ready.

Changelog

  • Patch
  • Minor
  • Skip

cdf

Fixed

  • Running cdf deploy for highly coupled views, the Toolkit now tries to recover if the API returns 400 with does not exists in the error message referencing a view that is part of the request.

templates

No changes.

Copy link

github-actions bot commented Mar 1, 2025

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
15983 11363 71% 60% 🟢

New Files

File Coverage Status
cognite_toolkit/_cdf_tk/utils/tarjan.py 100% 🟢
TOTAL 100% 🟢

Modified Files

File Coverage Status
cognite_toolkit/_cdf_tk/loaders/_resource_loaders/datamodel_loaders.py 85% 🟢
TOTAL 85% 🟢

updated for commit: 7efa870 by action🐍

Comment on lines -604 to -605
elif self._is_deployment_order(e1, set(self.get_ids(items))):
return self._fallback_create_one_by_one(self._topological_sort(items), e1, warn=False)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is handled by the new method, so we no longer need it.

@doctrino doctrino requested review from a team and removed request for a team March 3, 2025 07:28
@doctrino doctrino requested a review from BugGambit March 3, 2025 07:28
@BugGambit
Copy link

@erlendvollset what do you think here? Something that should be fixed in the API?

@doctrino
Copy link
Collaborator Author

doctrino commented Mar 3, 2025

@erlendvollset what do you think here? Something that should be fixed in the API?

Starbase is already notified, and yes this is definitely something that should be fixed in the API. However, we currently have a customers that is waiting for this fix. So suggest we merge this one, as that will solve the immediate problem.

@doctrino doctrino enabled auto-merge (squash) March 3, 2025 12:21
@doctrino doctrino merged commit ceaa1e5 into main Mar 3, 2025
12 checks passed
@doctrino doctrino deleted the try-3-view-dependency-orders branch March 3, 2025 15:04
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