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

Improve documentation and examples in example_asana.py #15959

Merged
merged 7 commits into from Jul 25, 2021

Conversation

jmelot
Copy link
Contributor

@jmelot jmelot commented May 20, 2021

Clarifies how to override workspace or project values from the connection in the operator parameters.

Copy link
Contributor

@ephraimbuddy ephraimbuddy left a comment

Choose a reason for hiding this comment

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

Should we add a system test for this example dag? Examples become more trusted when there's an accompanying system test. It will help us to also verify this example easily

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Jul 17, 2021
@jmelot jmelot force-pushed the improve-asana-example-dag-documentation branch from fe9a8ca to 2cea2d8 Compare July 18, 2021 20:10
@jmelot
Copy link
Contributor Author

jmelot commented Jul 19, 2021

Hi @ephraimbuddy, thanks for your comment above and sorry it was so long before I was able to get back to this. I added a system test. Please let me know if you have other suggestions or I need to make other modifications!

@uranusjr uranusjr removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Jul 19, 2021
Comment on lines 36 to 39
ASANA_TASK_TO_UPDATE = os.environ.get("ASANA_TASK_TO_UPDATE")
ASANA_TASK_TO_DELETE = os.environ.get("ASANA_TASK_TO_DELETE")
ASANA_PROJECT_ID = os.environ.get("ASANA_PROJECT_ID")
CONN_ID = os.environ.get("ASANA_CONNECTION_ID")
Copy link
Member

Choose a reason for hiding this comment

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

Is None an acceptable value for any of these? If not, it’s better to use os.environ["ASANA_TASK_TO_UPDATE"] etc. instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh good point -- done! (that also reminded me to add some comments describing how the connection should be set up as well)

Copy link
Contributor Author

@jmelot jmelot Jul 19, 2021

Choose a reason for hiding this comment

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

Actually, this leads to an import error when ci_prepare_provider_documentation.sh gets run -- switching back to os.environ.get for now, though will check around later and see if there's another way to resolve.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. Strange. Should not do that :)

Copy link
Member

Choose a reason for hiding this comment

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

can you point me to the error :) ?

Copy link
Contributor Author

@jmelot jmelot Jul 20, 2021

Choose a reason for hiding this comment

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

Hi @potiuk! This is the error I saw (tried to link to the relevant line but that doesn't seem to be working - it's line 449, I'll paste the stack trace below). I can revert my previous commit so this error will get triggered again, if that would be useful, or let me know anything else I can do to help.

  Traceback (most recent call last):
    File "/opt/airflow/dev/import_all_classes.py", line 77, in import_all_classes
      _module = importlib.import_module(modinfo.name)
    File "/usr/local/lib/python3.6/importlib/__init__.py", line 126, in 
  import_module
      return _bootstrap._gcd_import(name, package, level)
    File "<frozen importlib._bootstrap>", line 994, in _gcd_import
    File "<frozen importlib._bootstrap>", line 971, in _find_and_load
    File "<frozen importlib._bootstrap>", line 955, in _find_and_load_unlocked
    File "<frozen importlib._bootstrap>", line 665, in _load_unlocked
    File "<frozen importlib._bootstrap_external>", line 678, in exec_module
    File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
    File "/opt/airflow/airflow/providers/asana/example_dags/example_asana.py", 
  line 36, in <module>
      ASANA_TASK_TO_UPDATE = os.environ["ASANA_TASK_TO_UPDATE"]
    File "/usr/local/lib/python3.6/os.py", line 669, in __getitem__
      raise KeyError(key) from None
  KeyError: 'ASANA_TASK_TO_UPDATE'
  
  ----------------------------------------
  ########################################################################################################################
   [IN CONTAINER]   EXITING /opt/airflow/scripts/in_container/run_prepare_provider_documentation.sh WITH EXIT CODE 1  

@pytest.mark.system("asana")
class AsanaExampleDagsSystemTest(SystemTest):
def test_run_example_dag_asana(self):
self.run_dag("example_asana", DAG_FOLDER)
Copy link
Member

@uranusjr uranusjr Jul 19, 2021

Choose a reason for hiding this comment

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

We should probably refactor this a bit to avoid duplicate code. That’s something for another PR though.

@potiuk potiuk closed this Jul 19, 2021
@potiuk potiuk reopened this Jul 19, 2021
@potiuk
Copy link
Member

potiuk commented Jul 19, 2021

Rebuilding...

@potiuk potiuk merged commit e7bd82a into apache:main Jul 25, 2021
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

4 participants