Fix CLI connections import and migrate logic from secrets to Connection model #15425
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
natanweinberger
force-pushed
the
master
branch
3 times, most recently
from
420d632 to
c29a372
Compare
mik-laj
reviewed
Apr 19, 2021
natanweinberger
requested review from
XD-DENG,
ashb,
kaxil and
turbaszek
as code owners
Contributor
Author
|
I've refactored the functionality that reads data from a file and creates connections from it.
I was as conservative as possible in refactoring the secrets backend in this PR, but it seems like there's not much that uses it. I'd be open to doing some broader clean up there as a follow-up PR. |
natanweinberger
force-pushed
the
master
branch
7 times, most recently
from
52afacc to
5bce89d
Compare
natanweinberger
force-pushed
the
master
branch
3 times, most recently
from
5c675ba to
f8c5cdf
Compare
Contributor
Author
|
@mik-laj I've incorporated your feedback, can you please re-review when you have a chance? 🙏 |
natanweinberger
force-pushed
the
master
branch
4 times, most recently
from
3bfa59b to
a64c8f6
Compare
mik-laj
reviewed
May 4, 2021
natanweinberger
changed the title
natanweinberger
force-pushed
the
master
branch
20 times, most recently
from
51fd8a2 to
2fb1bd0
Compare
In connections_import, each connection was deserialized and stored into a Connection model instance rather than a dictionary, so an erroneous call to the dictionary methods .items() resulted in an AttributeError. With this fix, connection information is loaded from dictionaries directly into the Connection constructor and committed to the DB.
ashb
added a commit
that referenced
this pull request
Jun 22, 2021
…on model (#15425) * Add field 'extra' to Connection init * Fix connections import CLI In connections_import, each connection was deserialized and stored into a Connection model instance rather than a dictionary, so an erroneous call to the dictionary methods .items() resulted in an AttributeError. With this fix, connection information is loaded from dictionaries directly into the Connection constructor and committed to the DB. * Apply suggestions from code review * Use load_connections_dict in connections import Co-authored-by: Ash Berlin-Taylor <[email protected]> (cherry picked from commit 002075a)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a bug fix for the CLI command
connections_import. I originally added this CLI functionality in #15177. The bug was reported by @mudravrik here.Bug summary
Running
airflow connections import <filepath>results in an AttributeError.Root cause
I called the existing function
load_connections_dictto get the contents of the target file, thinking it returned a list of dictionaries. When I later looped through the list, I called the dictionary method.items()on each connection. However, each connection was actually stored into aConnectionmodel instance rather than a dictionary, so the call I made to.items()resulted in an AttributeError.Solution
In taking a closer look at this issue, I realized the code block that contained the bug can be removed altogether. Having a list of
Connectionmodels to begin with (rather than dictionaries) eliminated the need to even examine the keys and values of the dictionaries. I had only done that to safely load the data intoConnectionmodels in the first place.Preventing future issues
To avoid future issues, I also changed how the tests work for providing sample data. Before, a dictionary of sample data was set as the return value of
load_connections_dict, which actually returns an object of typeList[Connection]. Now, that dictionary of sample data is set to be the return value of a lower level function,_parse_secrets_file, which does no heavy lifting outside of parsing a JSON, YAML or env file and returns a dictionary.Bug originally introduced in #15177, which closed #9855.