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 2020/11/14 00:44:26 UTC

[GitHub] [airflow] fhoda opened a new pull request #12360: GCP Secrets Optional Lookup

fhoda opened a new pull request #12360:
URL: https://github.com/apache/airflow/pull/12360


   Optional lookup for connections, variables, and config for GCP Secrets Backend.
   
   Also cleaned up something in the tests for Azure optional lookup, and thought I'd just include it here.
   
   This is a followup to these PRs.
   #11736 - Vault
   #12143 - AWS
   #12174 - Azure
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] mik-laj commented on pull request #12360: GCP Secrets Optional Lookup

Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #12360:
URL: https://github.com/apache/airflow/pull/12360#issuecomment-730639715


   Fantastic contribution!  What are your plans for the next one?
   ![](https://www.gentletouchanimalhospital.com/wp-content/uploads/2019/10/happy-cat-1.jpg)


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] mik-laj commented on pull request #12360: GCP Secrets Optional Lookup

Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #12360:
URL: https://github.com/apache/airflow/pull/12360#issuecomment-727165268


   Can you also update guide? 
   https://airflow.readthedocs.io/en/latest/security/secrets/secrets-backend/google-cloud-secret-manager-backend.html


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] fhoda commented on pull request #12360: GCP Secrets Optional Lookup

Posted by GitBox <gi...@apache.org>.
fhoda commented on pull request #12360:
URL: https://github.com/apache/airflow/pull/12360#issuecomment-727238011


   > Can you also update guide?
   > https://airflow.readthedocs.io/en/latest/security/secrets/secrets-backend/google-cloud-secret-manager-backend.html
   
   Am I correct to assume you would like the guides to be updated for all the other Secret Backends, and not just the GCP Secrets Manager?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] fhoda commented on pull request #12360: GCP Secrets Optional Lookup

Posted by GitBox <gi...@apache.org>.
fhoda commented on pull request #12360:
URL: https://github.com/apache/airflow/pull/12360#issuecomment-730592596


   Thank you again!  @mik-laj & @kaxil 


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] github-actions[bot] commented on pull request #12360: GCP Secrets Optional Lookup

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12360:
URL: https://github.com/apache/airflow/pull/12360#issuecomment-727165115


   The PR should be OK to be merged with just subset of tests as it does not modify Core of Airflow. The committers might merge it or can add a label 'full tests needed' and re-run it to run all tests if they see it is needed!


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] fhoda commented on pull request #12360: GCP Secrets Optional Lookup

Posted by GitBox <gi...@apache.org>.
fhoda commented on pull request #12360:
URL: https://github.com/apache/airflow/pull/12360#issuecomment-727642399


   Went ahead and updated all of the the relevant guides. "Feature parity" :) 


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] kaxil merged pull request #12360: GCP Secrets Optional Lookup

Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #12360:
URL: https://github.com/apache/airflow/pull/12360


   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] fhoda commented on a change in pull request #12360: GCP Secrets Optional Lookup

Posted by GitBox <gi...@apache.org>.
fhoda commented on a change in pull request #12360:
URL: https://github.com/apache/airflow/pull/12360#discussion_r523304249



##########
File path: airflow/providers/google/cloud/secrets/secret_manager.py
##########
@@ -89,11 +92,12 @@ def __init__(
         self.variables_prefix = variables_prefix
         self.config_prefix = config_prefix
         self.sep = sep
-        if not self._is_valid_prefix_and_sep():
-            raise AirflowException(
-                "`connections_prefix`, `variables_prefix` and `sep` should "
-                f"follows that pattern {SECRET_ID_PATTERN}"
-            )
+        if connections_prefix is not None:
+            if not self._is_valid_prefix_and_sep():
+                raise AirflowException(
+                    "`connections_prefix`, `variables_prefix` and `sep` should "
+                    f"follows that pattern {SECRET_ID_PATTERN}"
+                )

Review comment:
       This one is a little different from the others, as it checks if the prefix and sep are valid. But the function `_is_valid_prefix_and_sep` only seems to really check the `connections_prefix` with `sep`, and not `variables_prefix` or `config_prefix`. So the added condition reflects this behavior.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org