You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/09/23 03:16:44 UTC
[GitHub] [airflow] josh-fell opened a new pull request #18456: Proper handling of Account URL custom conn field in AzureBatchHook
josh-fell opened a new pull request #18456:
URL: https://github.com/apache/airflow/pull/18456
Closes: #18442
The existing `_get_required_params()` method is passed an invalid value for the Batch Account URL and will always raise an exception for this missing required parameter. This value comes from the classic `Extra` field in the connection form which is no longer exposed in the Azure Batch Service connection type.
Additionally there are minor updates made to the hook's docstring for information accuracy, small spelling/grammar changes in the `AzureBatchHook`, updated the corresponding connection doc, and cleaned up the relevant unit test.
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal merged pull request #18456: Proper handling of Account URL custom conn field in AzureBatchHook
Posted by GitBox <gi...@apache.org>.
eladkal merged pull request #18456:
URL: https://github.com/apache/airflow/pull/18456
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] josh-fell edited a comment on pull request #18456: Proper handling of Account URL custom conn field in AzureBatchHook
Posted by GitBox <gi...@apache.org>.
josh-fell edited a comment on pull request #18456:
URL: https://github.com/apache/airflow/pull/18456#issuecomment-925488306
The failing CI tests succeed locally and it looks like the tests are not executing the updated unit test code from this PR which is why they are throwing exceptions.
For example with the Sqlite, Py3.6 test these are the results running locally:
Here is an example of outdated code relative to this PR:
**Executed code**
```python
# connect with vm configuration
db.merge_conn(
Connection(
conn_id=self.test_vm_conn_id,
conn_type="azure_batch",
extra=json.dumps(
{
"account_name": self.test_account_name,
"account_key": self.test_account_key,
"account_url": self.test_account_url,
"vm_publisher": self.test_vm_publisher,
"vm_offer": self.test_vm_offer,
"vm_sku": self.test_vm_sku,
"node_agent_sku_id": self.test_node_agent_sku,
}
),
)
)
```
**Updated code in PR**
```python
# connect with vm configuration
db.merge_conn(
Connection(
conn_id=self.test_vm_conn_id,
conn_type="azure_batch",
extra=json.dumps({"extra__azure_batch__account_url": self.test_account_url}),
)
)
```
Happy to close/reopen this PR if you think that will do the trick but maybe I'm missing something obvious.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #18456: Proper handling of Account URL custom conn field in AzureBatchHook
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #18456:
URL: https://github.com/apache/airflow/pull/18456#issuecomment-925882798
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] josh-fell commented on pull request #18456: Proper handling of Account URL custom conn field in AzureBatchHook
Posted by GitBox <gi...@apache.org>.
josh-fell commented on pull request #18456:
URL: https://github.com/apache/airflow/pull/18456#issuecomment-925488306
The failing CI tests succeed locally and it looks like the tests are not executing the updated unit test code from this PR which is why they are throwing exceptions.
For example with the Sqlite, Py3.6 test these are the results running locally:
Here is an example of outdated code relative to this PR:
```python
# connect with vm configuration
db.merge_conn(
Connection(
conn_id=self.test_vm_conn_id,
conn_type="azure_batch",
extra=json.dumps(
{
"account_name": self.test_account_name,
"account_key": self.test_account_key,
"account_url": self.test_account_url,
"vm_publisher": self.test_vm_publisher,
"vm_offer": self.test_vm_offer,
"vm_sku": self.test_vm_sku,
"node_agent_sku_id": self.test_node_agent_sku,
}
),
)
)
```
Happy to close/reopen this PR if you think that will do the trick.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on pull request #18456: Proper handling of Account URL custom conn field in AzureBatchHook
Posted by GitBox <gi...@apache.org>.
eladkal commented on pull request #18456:
URL: https://github.com/apache/airflow/pull/18456#issuecomment-925564039
Used the code in your branch and tests are failing (like they do in the CI)
In the PR you are changing only the hook. Shouldn't the operator test connection be changed as well?
https://github.com/apache/airflow/blob/6dbeb82eef263f5f56b46fbb4a9140a288bddd23/tests/providers/microsoft/azure/operators/test_azure_batch.py#L63-L96
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] josh-fell commented on pull request #18456: Proper handling of Account URL custom conn field in AzureBatchHook
Posted by GitBox <gi...@apache.org>.
josh-fell commented on pull request #18456:
URL: https://github.com/apache/airflow/pull/18456#issuecomment-925719102
Yes, you're right. That's a miss and is the obvious issue if I actually read the error message properly.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org