Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AIRFLOW-6399] Serialization: DAG access_control field should be deco… #7374

Closed
wants to merge 1 commit into from

Conversation

codenamelxl
Copy link

@codenamelxl codenamelxl commented Feb 6, 2020

'_access_control' field need to be defined as decorated field so the field can be serialized/deserialized incorrectly.


Issue link: AIRFLOW-6399

Make sure to mark the boxes below before creating PR: [x]

  • Description above provides context of the change
  • Commit message/PR title starts with [AIRFLOW-6399].
  • Unit tests coverage for changes (not needed for documentation changes)
  • Commits follow "How to write a good git commit message"
  • Relevant documentation is updated including usage instructions.
  • I will engage committers as explained in Contribution Workflow Example.

In case of fundamental code change, 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 UPDATING.md.
Read the Pull Request Guidelines for more information.

@boring-cyborg
Copy link

boring-cyborg bot commented Feb 6, 2020

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
Here are some useful points:

  • Pay attention to the quality of your code (flake8, pylint and type annotations). Our pre-commits will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
  • Consider using Breeze environment for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://apache-airflow-slack.herokuapp.com/

@codenamelxl codenamelxl changed the title [AIRFLOW_6399] Serialization: DAG access_control field should be deco… [AIRFLOW-6399] Serialization: DAG access_control field should be deco… Feb 6, 2020
@codenamelxl
Copy link
Author

Failed check is about PR title. I have fixed the title but not sure how to rerun the check.

@potiuk
Copy link
Member

potiuk commented Feb 6, 2020

Failed check is about PR title. I have fixed the title but not sure how to rerun the check.

You need to amend the commit - it's the commit title that is a problem"

Wrong commit title: [AIRFLOW_6399] Serialization: DAG access

@codenamelxl
Copy link
Author

Failed check is about PR title. I have fixed the title but not sure how to rerun the check.

You need to amend the commit - it's the commit title that is a problem"

Wrong commit title: [AIRFLOW_6399] Serialization: DAG access

Oh, silly me. Fixed. Thanks.

@codecov-io
Copy link

codecov-io commented Feb 7, 2020

Codecov Report

Merging #7374 into master will decrease coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #7374      +/-   ##
=========================================
- Coverage   86.35%   86.3%   -0.06%     
=========================================
  Files         871     871              
  Lines       40627   40660      +33     
=========================================
+ Hits        35084   35092       +8     
- Misses       5543    5568      +25
Impacted Files Coverage Δ
airflow/serialization/serialized_objects.py 90.16% <100%> (ø) ⬆️
...w/providers/apache/hive/operators/mysql_to_hive.py 100% <0%> (ø) ⬆️
airflow/kubernetes/volume_mount.py 44.44% <0%> (-55.56%) ⬇️
airflow/kubernetes/volume.py 52.94% <0%> (-47.06%) ⬇️
airflow/security/kerberos.py 76.08% <0%> (ø) ⬆️
airflow/kubernetes/pod_launcher.py 47.18% <0%> (-45.08%) ⬇️
airflow/providers/mysql/operators/mysql.py 100% <0%> (ø) ⬆️
airflow/kubernetes/refresh_config.py 50.98% <0%> (-23.53%) ⬇️
...viders/cncf/kubernetes/operators/kubernetes_pod.py 70.21% <0%> (-23.41%) ⬇️
airflow/api/common/experimental/__init__.py 92.59% <0%> (-7.41%) ⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e150cf...d9a1565. Read the comment docs.

@kaxil
Copy link
Member

kaxil commented Feb 7, 2020

Can you add a new test or update the ground_truth_dag with access_control

@codenamelxl
Copy link
Author

Can you add a new test or update the ground_truth_dag with access_control

"access_control" added to ground_truth_dag. Thank for guiding me.

@kaxil
Copy link
Member

kaxil commented Feb 10, 2020

Still not able to see the test, maybe you forgot to push?

Btw I am OOO until 18 Feb, so please bear me with me if I am not able to recheck the PR till then

@codenamelxl
Copy link
Author

@kaxil Test run OK with Postgres. I'm not sure what wrong with the test for MySQL and SQLite.

@kaxil
Copy link
Member

kaxil commented Mar 12, 2020

I have restarted the test

@lsimoneau
Copy link
Contributor

The test output is hard to read, but I think this is failing because can_dag_edit and can_dag_read are not in a consistent order on retrieval from the database. I've got the test failure reproduced locally and I'm trying to fix this today, will open a new PR if I get it working.

@codenamelxl
Copy link
Author

codenamelxl commented Mar 26, 2020

The test output is hard to read, but I think this is failing because can_dag_edit and can_dag_read are not in a consistent order on retrieval from the database. I've got the test failure reproduced locally and I'm trying to fix this today, will open a new PR if I get it working.

You're right. And seems like your PR test pass. I would be happy to see this change merged 👍

@kaxil kaxil closed this Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants