Skip to content

Fix displaying dbt docs as menu item in Astro#1280

Merged
tatiana merged 6 commits into
mainfrom
fix-1131
Oct 24, 2024
Merged

Fix displaying dbt docs as menu item in Astro#1280
tatiana merged 6 commits into
mainfrom
fix-1131

Conversation

@tatiana
Copy link
Copy Markdown
Collaborator

@tatiana tatiana commented Oct 23, 2024

As of Cosmos 1.7.0, Astro customers who have the role of "Organization Member" or "Workspace Operator" are not able to see the menu item "dbt Docs" under "Browser". They could access the /cosmos/dbt_docs view directly, though. This PR solves this issue.

Closes: #1131

About the solution

This PR follows the Astro documentation recommendation:

If you want to use custom menu items in an Airflow environment hosted on Astro, you must give your plugin the necessary permissions. To do this, use the @has_access decorator to give your BaseView method ACTION_CAN_ACCESS_MENU permissions.

By adding:

    @has_access([(permissions.ACTION_CAN_ACCESS_MENU, "Custom Menu")]) 

How the fix was tested

In addition to the unit test that checks if all Cosmos plugin endpoints have the expected permissions, I also validated this change with a user with the "Organization Member" role. I confirmed that they could see the menu item and that it redirects to the correct page:

Screenshot 2024-10-24 at 10 24 03

@netlify
Copy link
Copy Markdown

netlify Bot commented Oct 23, 2024

Deploy Preview for sunny-pastelito-5ecb04 failed.

Name Link
🔨 Latest commit 48c5f3b
🔍 Latest deploy log https://app.netlify.com/sites/sunny-pastelito-5ecb04/deploys/671a7da6c3d0ab0008aadb96

@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.73%. Comparing base (7915fd0) to head (48c5f3b).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1280   +/-   ##
=======================================
  Coverage   95.73%   95.73%           
=======================================
  Files          67       67           
  Lines        3965     3967    +2     
=======================================
+ Hits         3796     3798    +2     
  Misses        169      169           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tatiana tatiana marked this pull request as ready for review October 24, 2024 17:03
@dosubot dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. area:plugin Related to the Cosmos AirflowPlugin. area:testing Related to testing, like unit tests, integration tests, etc labels Oct 24, 2024
Copy link
Copy Markdown
Contributor

@pankajkoti pankajkoti left a comment

Choose a reason for hiding this comment

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

The Airflow docstring suggests that the has_access decorator is deprecated and we should use alternate has_access_* (probably has_access_view is what we might need) decorators. We might as well need update to the Astro docs(?)

Screenshot 2024-10-24 at 11 11 58 PM

However, since this has been tested with Astro & it seems to be working, I guess we're safe.

Great find though, thanks for fixing this & sharing the Astro docs for reference 👏🏽

@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Oct 24, 2024
@tatiana
Copy link
Copy Markdown
Collaborator Author

tatiana commented Oct 24, 2024

Thanks a lot for the fast review, @pankajkoti!

The Airflow docstring suggests that the has_access decorator is deprecated and we should use alternate has_access_* (probably has_access_view is what we might need) decorators. We might as well need update to the Astro docs(?)

I also noticed this but was slightly worried about refactoring this while trying to fix the current bug.

It was a significant interface change. While has_access receives a list of tuples (permission & resource) as an argument, the new interface (e.g. has_access_view) expects an AccessView item. It is unclear if has_access_view would be enough to grant menu item permissions:
https://github.com/apache/airflow/blob/6070bb6c354908f6b233f3d92086476d0f62ada0/airflow/www/auth.py#L324

Lastly, I wonder how much of this may change with Airflow 3. I know there are intentions of redefining plugins, so I'm not sure if we should spend time on this now or just focus on the future version once AF3 alphas are released. WDYT?

However, since this has been tested with Astro and seems to be working, I guess we're safe.

I also asked the CRE team to confirm it works in their tests so that we can have more confidence it now works as expected in Astro Cloud. I will merge this PR, but I suggest we wait for their confirmation before releasing 1.7.1.

@pankajkoti
Copy link
Copy Markdown
Contributor

pankajkoti commented Oct 24, 2024

Lastly, I wonder how much of this may change with Airflow 3. I know there are intentions of redefining plugins, so I'm not sure if we should spend time on this now or just focus on the future once AF3 alphas are released. WDYT?

I fully agree. IMO, it would be wise if we invest little into trying to support more in what we may know is likely to change. So the bare minimum and working fixes is what I would also prefer specifically when it comes to plugins ,🙂

@tatiana tatiana merged commit 342ce3a into main Oct 24, 2024
@tatiana tatiana deleted the fix-1131 branch October 24, 2024 18:46
@tatiana tatiana added this to the Cosmos 1.7.1 milestone Oct 25, 2024
tatiana added a commit that referenced this pull request Oct 29, 2024
Bug fixes

* Fix ``DbtVirtualenvBaseOperator`` to use correct virtualenv Python path by kesompochy in #1252
* Fix displaying dbt docs as menu item in Astro by @tatiana in #1280
* Fix: Replace login by user for clickhouse profile by @petershenri in #1255

Enhancements

* Improve dbt Docs Hosting Debugging -- Update dbt_docs_not_set_up.html by @johnmcochran in #1250
* Minor refactor on VirtualenvOperators & add test for PR by @tatiana in #1253

Docs

* Add Welcome Section and "What Is Cosmos" Blurb to Home Page by @cmarteepants and @yanmastin-astro in #1251
* Update the URL for sample dbt docs hosted in Astronomer S3 bucket by @pankajkoti in #1283
* Add dedicated scarf tracking pixel to readme by @cmarteepants in #1256

Others

* Update ``CODEOWNERS`` to track all files by @pankajkoti in #1284
* Fix release after the ``raw`` rst directive disabled was disabled in PyPI by @tatiana in #1282
* Update issue template ``bug.yml`` - cosmos version update in the dropdown by @pankajkoti in #1275
* Pre-commit hook updates in #1285, #1274, #1254, #1244"
error: pathspec 'Is' did not match any file(s) known to git
error: pathspec 'Cosmos Blurb to Home Page by @cmarteepants and @yanmastin-astro in #1251
* Update the URL for sample dbt docs hosted in Astronomer S3 bucket by @pankajkoti in #1283
* Add dedicated scarf tracking pixel to readme by @cmarteepants in #1256

Others

* Update CODEOWNERS to track all files by @pankajkoti in #1284
* Fix release after the raw rst directive disabled was disabled in PyPI by @tatiana in #1282
* Update issue template bug.yml - cosmos version update in the dropdown by @pankajkoti in #1275
* Pre-commit hook updates in #1285, #1274, #1254, #1244
@tatiana tatiana mentioned this pull request Oct 29, 2024
tatiana added a commit that referenced this pull request Oct 29, 2024
Bug fixes

* Fix ``DbtVirtualenvBaseOperator`` to use correct virtualenv Python path by kesompochy in #1252
* Fix displaying dbt docs as menu item in Astro by @tatiana in #1280
* Fix: Replace login by user for clickhouse profile by @petershenri in #1255

Enhancements

* Improve dbt Docs Hosting Debugging -- Update dbt_docs_not_set_up.html by @johnmcochran in #1250
* Minor refactor on VirtualenvOperators & add test for PR by @tatiana in #1253

Docs

* Add Welcome Section and "What Is Cosmos" Blurb to Home Page by @cmarteepants and @yanmastin-astro in #1251
* Update the URL for sample dbt docs hosted in Astronomer S3 bucket by @pankajkoti in #1283
* Add dedicated scarf tracking pixel to readme by @cmarteepants in #1256

Others

* Update ``CODEOWNERS`` to track all files by @pankajkoti in #1284
* Fix release after the ``raw`` rst directive disabled was disabled in PyPI by @tatiana in #1282
* Update issue template ``bug.yml`` - cosmos version update in the dropdown by @pankajkoti in #1275
* Pre-commit hook updates in #1285, #1274, #1254, #1244"
error: pathspec 'Is' did not match any file(s) known to git
error: pathspec 'Cosmos Blurb to Home Page by @cmarteepants and @yanmastin-astro in #1251
* Update the URL for sample dbt docs hosted in Astronomer S3 bucket by @pankajkoti in #1283
* Add dedicated scarf tracking pixel to readme by @cmarteepants in #1256

Others

* Update CODEOWNERS to track all files by @pankajkoti in #1284
* Fix release after the raw rst directive disabled was disabled in PyPI by @tatiana in #1282
* Update issue template bug.yml - cosmos version update in the dropdown by @pankajkoti in #1275
* Pre-commit hook updates in #1285, #1274, #1254, #1244
tatiana added a commit that referenced this pull request Oct 29, 2024
Bug fixes

* Fix ``DbtVirtualenvBaseOperator`` to use correct virtualenv Python
path by @kesompochy in #1252
* Fix displaying dbt docs as menu item in Astro by @tatiana in #1280
* Fix: Replace login by user for clickhouse profile by @petershenri in
#1255

Enhancements

* Improve dbt Docs Hosting Debugging -- Update dbt_docs_not_set_up.html
by @johnmcochran in #1250
* Minor refactor on VirtualenvOperators & add test for PR by @tatiana in
#1253

Docs

* Add Welcome Section and "What Is Cosmos" Blurb to Home Page by
@cmarteepants and @yanmastin-astro in #1251
* Update the URL for sample dbt docs hosted in Astronomer S3 bucket by
@pankajkoti in #1283
* Add a dedicated scarf tracking pixel to readme by @cmarteepants in #1256

Others

* Update ``CODEOWNERS`` to track all files by @pankajkoti in #1284
* Fix release after the ``raw`` rst directive disabled was disabled in
PyPI by @tatiana in #1282
* Update issue template ``bug.yml`` - cosmos version update in the
dropdown by @pankajkoti in #1275
* Pre-commit hook updates in #1285, #1274, #1254, #1244
tatiana added a commit that referenced this pull request Nov 11, 2024
Since Cosmos 1.7.1, users who do not use Astro started getting 'Access is Denied' when trying to access the dbt docs menu item.

This was an uninted side effect of the fix for Astro users to be able to see that menu item, introduced in: #1280

Closes: #1309
tatiana added a commit that referenced this pull request Nov 11, 2024
Since Cosmos 1.7.1, users who do not use Astro started getting 'Access is Denied' when trying to access the dbt docs menu item.

This was an uninted side effect of the fix for Astro users to be able to see that menu item, introduced in: #1280

Closes: #1309
tatiana added a commit that referenced this pull request Nov 12, 2024
Since Cosmos 1.7.1, users who do not use Astro Cloud started facing
'Access is Denied' when accessing the dbt docs menu item.

This was an unintended side effect of the fix for Astro users to be able
to see that menu item, introduced in #1280

Closes: #1309
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:plugin Related to the Cosmos AirflowPlugin. area:testing Related to testing, like unit tests, integration tests, etc lgtm This PR has been approved by a maintainer size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] User unable to see the "dbt docs" menu item in Astro

2 participants