Skip to content
This repository was archived by the owner on Sep 4, 2024. It is now read-only.

Pass api version to jobs API calls across operator implementations #66

Merged
merged 6 commits into from
Mar 20, 2024
Merged

Pass api version to jobs API calls across operator implementations #66

merged 6 commits into from
Mar 20, 2024

Conversation

pankajkoti
Copy link
Collaborator

@pankajkoti pankajkoti commented Mar 19, 2024 •
edited
Loading

Based on a user feedback, they would like to use
Databricks API version 2.1 for the various Databricks SDK
calls from the operator implementations. Hence, use the existing
configurable API version constant fetched from an
environment variable and pass it in places where
Databricks API calls are made from the Databricks SDK
across the operators implementations.

closes: https://github.com/astronomer/issues-airflow/issues/614

@pankajkoti pankajkoti force-pushed the pass-jobs-api-version branch from 10af94e to 16759dc Compare March 20, 2024 10:13
@pankajkoti pankajkoti force-pushed the pass-jobs-api-version branch from 16759dc to 9af597f Compare March 20, 2024 10:20
@pankajkoti pankajkoti marked this pull request as ready for review March 20, 2024 11:17
@pankajkoti pankajkoti changed the title Pass jobs api version Pass api version to jobs API calls across operator implementations Mar 20, 2024
phanikumv
phanikumv reviewed Mar 20, 2024
View reviewed changes
Copy link

@phanikumv phanikumv left a comment

Choose a reason for hiding this comment

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

dev/logs/scheduler/latest -- is this intentionally removed?

@pankajkoti
Copy link
Collaborator Author

pankajkoti commented Mar 20, 2024 •
edited
Loading

dev/logs/scheduler/latest -- is this intentionally removed?

yes, looks like the log file was pushed to git erroneously earlier. Hence, removed it from Git tracking.

@pankajkoti pankajkoti requested a review from phanikumv March 20, 2024 11:51
phanikumv
phanikumv reviewed Mar 20, 2024
View reviewed changes
kaxil
kaxil approved these changes Mar 20, 2024
View reviewed changes
@pankajkoti pankajkoti merged commit 0fce468 into main Mar 20, 2024
25 checks passed
@pankajkoti pankajkoti deleted the pass-jobs-api-version branch March 20, 2024 12:15
tatiana
tatiana reviewed Mar 20, 2024
View reviewed changes
Copy link
Collaborator

@tatiana tatiana left a comment

Choose a reason for hiding this comment

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

@pankajkoti it looks good overall!

Some feedback:

  1. It seems we don't have any tests that confirm if users set an environment variable, it will be forwarded to the API calls - which I understood is the goal with this ticket
  2. Were we exposing this environment variable before? It feels that JOBS_API_VERSION can be misleading - if we could expose it as DATABRICKS_JOBS_API_VERSION
  3. This was there before, but since we're using this as a setting, and not really a constant, it would be great if we had setttings.py as opposed to constants.py

.github/workflows/ci.yml
name: Build and test astro databricks provider
on:
on: # yamllint disable-line rule:truthy
Copy link
Collaborator

Choose a reason for hiding this comment

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

@pankajkoti Curious about this comment!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

YAML linter was complaining that the value needs to be True or False. This is a way we have been using for the YAML linter in GitHub CI workflows across other projects to for disabling this false lint report.

@pankajkoti
Copy link
Collaborator Author

pankajkoti commented Mar 20, 2024 •
edited
Loading

@pankajkoti it looks good overall!

Some feedback:

  1. It seems we don't have any tests that confirm if users set an environment variable, it will be forwarded to the API calls - which I understood is the goal with this ticket

The constant JOBS_API_VERSION that fetches this environment variable takes a default value if an env variable is not set. And thus we have a set value that gets passed to the API calls. The ticket was that although we had this constant, it was not getting passed across all the API calls and this is what we are trying to support in the PR.

  1. Were we exposing this environment variable before? It feels that JOBS_API_VERSION can be misleading - if we could expose it as DATABRICKS_JOBS_API_VERSION

Happy to rename it to so. Will create an immediate follow up PR.

  1. This was there before, but since we're using this as a setting, and not really a constant, it would be great if we had setttings.py as opposed to constants.py

Yes, makes sense. Will create an immediate follow up PR.

Edit: Follow up PR to address the comments #68

pankajkoti added a commit that referenced this pull request Mar 20, 2024
@pankajkoti
Address follow up comments on PR #66
edef7b3
pankajkoti added a commit that referenced this pull request Mar 20, 2024
@pankajkoti
Address follow up comments on PR #66 (#68)
22aba73 
The commit includes following changes:
- Rename JOBS_API_VERSION to DATABRICKS_JOBS_API_VERSION
- Move constants.py to settings.py

These changes are based on review comments in PR #66
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tatiana tatiana tatiana left review comments

@phanikumv phanikumv phanikumv approved these changes

@dimberman dimberman Awaiting requested review from dimberman dimberman is a code owner

@pankajastro pankajastro Awaiting requested review from pankajastro

@kaxil kaxil Awaiting requested review from kaxil

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pankajkoti @tatiana @kaxil @phanikumv @pankajastro