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

fix(datajobs): fetch dataflow properties from a relationship #3487

Merged

Conversation

gabe-lyons
Copy link
Contributor

Introduced in: #3357.

Historically, we have not been fetching data flows via relationships. Recently, that changed when the dataFlow field was deprecated in #3278. In this #3357, that logic was replaced with a shared relationship fragment which had not yet had a select clause for data flows added to it.

Followups:

  1. I added a unit test to load this section. This unit test would not have caught this error because the apollo mock doens't take into account what fields are being selected. However, more test coverage on high risk areas is better.

  2. This kind of error was sneaky because it only came up if someone navigating specifically to the Pipeline tab of a Data Task. UI integration tests should make sure to open each tab of each entity, rather than just the main page.

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable)

@github-actions
Copy link

Unit Test Results

     58 files  ±0       58 suites  ±0   21m 12s ⏱️ - 1h 31m 18s
   525 tests ±0     473 ✔️ +1  52 💤 ±0  0  - 1 
1 174 runs  ±0  1 106 ✔️ +1  68 💤 ±0  0  - 1 

Results for commit 3da70c7. ± Comparison against base commit 1fec105.

</Link>
<TagContainer>
<TagTermGroup uneditableGlossaryTerms={glossaryTerms} uneditableTags={tags} maxShow={3} />
</TagContainer>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these changes are to remove warnings in CI

Copy link
Contributor

@shirshanka shirshanka left a comment

Choose a reason for hiding this comment

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

LGTM

@shirshanka shirshanka merged commit c1ca297 into datahub-project:master Oct 29, 2021
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.

2 participants