Re: [PR] Update ACL during job reset [airflow]
pankajkoti merged PR #38741: URL: https://github.com/apache/airflow/pull/38741 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Update ACL during job reset [airflow]
Lee-W commented on code in PR #38741: URL: https://github.com/apache/airflow/pull/38741#discussion_r1557069145 ## airflow/providers/databricks/operators/databricks.py: ## @@ -316,6 +316,10 @@ def execute(self, context: Context) -> int: if job_id is None: return self._hook.create_job(self.json) self._hook.reset_job(str(job_id), self.json) +if "access_control_list" in self.json and self.json["access_control_list"] is not None: Review Comment: It's indeed even better -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Update ACL during job reset [airflow]
Taragolis commented on code in PR #38741:
URL: https://github.com/apache/airflow/pull/38741#discussion_r1557056585
##
airflow/providers/databricks/operators/databricks.py:
##
@@ -316,6 +316,10 @@ def execute(self, context: Context) -> int:
if job_id is None:
return self._hook.create_job(self.json)
self._hook.reset_job(str(job_id), self.json)
+if "access_control_list" in self.json and
self.json["access_control_list"] is not None:
Review Comment:
Python 3.8+ there
```python
if (access_control_list := self.json.get("access_control_list")) is not None:
acl_json = {"access_control_list": access_control_list}
self._hook.update_job_permission(normalise_json_content(acl_json))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Update ACL during job reset [airflow]
Lee-W commented on code in PR #38741: URL: https://github.com/apache/airflow/pull/38741#discussion_r1557042849 ## airflow/providers/databricks/operators/databricks.py: ## @@ -316,6 +316,10 @@ def execute(self, context: Context) -> int: if job_id is None: return self._hook.create_job(self.json) self._hook.reset_job(str(job_id), self.json) +if "access_control_list" in self.json and self.json["access_control_list"] is not None: Review Comment: Yep, i think this is better but I noticed this practice is used elsewhere. Should we change all of them into this style? (maybe create an issue?) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Update ACL during job reset [airflow]
pankajkoti commented on code in PR #38741:
URL: https://github.com/apache/airflow/pull/38741#discussion_r1556969732
##
airflow/providers/databricks/operators/databricks.py:
##
@@ -316,6 +316,10 @@ def execute(self, context: Context) -> int:
if job_id is None:
return self._hook.create_job(self.json)
self._hook.reset_job(str(job_id), self.json)
+if "access_control_list" in self.json and
self.json["access_control_list"] is not None:
Review Comment:
```suggestion
if self.json.get("access_control_list") is not None:
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Update ACL during job reset [airflow]
subham611 commented on code in PR #38741: URL: https://github.com/apache/airflow/pull/38741#discussion_r1556066734 ## airflow/providers/databricks/hooks/databricks.py: ## @@ -655,6 +656,16 @@ def get_repo_by_path(self, path: str) -> str | None: return None +def update_job_permission(self, json: dict[str, Any]) -> dict: +""" +Update databricks job permission + +:param json: payload +:return: Review Comment: added. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Update ACL during job reset [airflow]
Lee-W commented on code in PR #38741: URL: https://github.com/apache/airflow/pull/38741#discussion_r1555946331 ## airflow/providers/databricks/hooks/databricks.py: ## @@ -655,6 +656,16 @@ def get_repo_by_path(self, path: str) -> str | None: return None +def update_job_permission(self, json: dict[str, Any]) -> dict: +""" +Update databricks job permission + +:param json: payload +:return: Review Comment: Is the content missing? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Update ACL during job reset [airflow]
Lee-W commented on PR #38741: URL: https://github.com/apache/airflow/pull/38741#issuecomment-2042912496 Thanks @potiuk for help out! @SubhamSinghal There's one static check failure. We might need your help to fix it -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Update ACL during job reset [airflow]
potiuk commented on PR #38741: URL: https://github.com/apache/airflow/pull/38741#issuecomment-2042844353 Rebased it -> your PR was 62 commits behind -> next time you can do it yourself too (see our contributing docs and search for rebase) https://github.com/apache/airflow/blob/main/contributing-docs/10_working_with_git.rst -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Update ACL during job reset [airflow]
subham611 commented on PR #38741: URL: https://github.com/apache/airflow/pull/38741#issuecomment-2042504136 @Lee-W How to fix `Wait for CI images` error. Need help here. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Update ACL during job reset [airflow]
Lee-W commented on PR #38741: URL: https://github.com/apache/airflow/pull/38741#issuecomment-2041477480 > @Lee-W `Wait for CI images ` step is failing. How should I fix this, or simply rerun would work? Let me trigger rerun and see how it works -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Update ACL during job reset [airflow]
subham611 commented on PR #38741: URL: https://github.com/apache/airflow/pull/38741#issuecomment-2041350618 @Lee-W `Wait for CI images ` step is failing. How should I fix this, or simply rerun would work? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Update ACL during job reset [airflow]
SubhamSinghal commented on PR #38741: URL: https://github.com/apache/airflow/pull/38741#issuecomment-2036767775 @Lee-W Please review changes. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
