Skip to content

Conversation

@potiuk
Copy link
Member

@potiuk potiuk commented Dec 5, 2022

Introduce better description and significant change in release notes for import limiting for deserialization.

Follow up after #27887


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

Introduce better description and significant change in release
notes for import limiting for deserialization.

Follow up after apache#27887
@bolkedebruin
Copy link
Contributor

The serializer will be generic and not just tied to XCom so referring to XCom doesn't make sense

@potiuk
Copy link
Member Author

potiuk commented Dec 5, 2022

The serializer will be generic and not just tied to XCom so referring to XCom doesn't make sense

But we can change the message then ? can we? For now it's only XCom, as I understand.

@potiuk
Copy link
Member Author

potiuk commented Dec 5, 2022

But we can change the message then ? can we? For now it's only XCom, as I understand.

Just some reason why - It took me quite a while to look it up (going through Git History and finding PR) so I can only imagine how hard it might be for the user, and I think we should give as good a hint where it is from as we can.

@potiuk
Copy link
Member Author

potiuk commented Dec 5, 2022

But yeah. anyhow let's wait for the user to respond how he got there, I am also curious why it was triggered at all.

@bolkedebruin
Copy link
Contributor

This is the follow up #28067

raise ImportError(
f"{classname} was not found in allow list for import in XCom. "
"If you want to continue to use your class in XCom, add it to "
"allowed_deserialization_classes config in core section of config."
Copy link
Member

@kaxil kaxil Dec 5, 2022

Choose a reason for hiding this comment

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

Suggested change
"allowed_deserialization_classes config in core section of config."
"allowed_deserialization_classes config in core section of airflow.cfg."

if not cls:
raise ImportError(f"{classname} was not found in allow list for import")
raise ImportError(
f"{classname} was not found in allow list for import in XCom. "
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
f"{classname} was not found in allow list for import in XCom. "
f"{classname} was not found in allow list for imports. "

@bolkedebruin
Copy link
Contributor

As I intend to further refactor it in the earlier mentioned PR and in the intention is to have that out as part of 2.5.1 and this is just clarification I think I'd better solve it there.

@potiuk
Copy link
Member Author

potiuk commented Dec 5, 2022

As I intend to further refactor it in the earlier mentioned PR and in the intention is to have that out as part of 2.5.1 and this is just clarification I think I'd better solve it there.

Cool. Closing it then :)

@potiuk potiuk closed this Dec 5, 2022
@potiuk potiuk deleted the add-better-description-of-allowed-imports branch June 4, 2023 01:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants