-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
Open relative extra links in place #17477
Open relative extra links in place #17477
Conversation
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/main/CONTRIBUTING.rst)
|
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease. |
(closed/reopened to rekick CI) |
@jedcunningham (i noticed that you re-triggered the CI build). The build is failing due to a failed build constraint:
I believe this is unrelated to my change, as a clean build failed even before I made any changes and I didn't change any dependencies. However, I haven't had the chance to track down the root of the issue yet. I was hoping that if I gave it a while, a fix to the root cause might be merged in allowing my change to then build successfully. Does that sound like a reasonable approach? Or should I try to hunt down the cause? |
Sorry, I should have looked more closely. Since main isn't currently failing, can you try rebasing on main? You probably waited long enough 👍 |
267eec8
to
88eb5ca
Compare
Done. Will keep an eye on the build. |
@bradarndt, looks like 2 static check failures, both should be easy to fix. I'd also suggest that you set up the pre-commit hooks, they help catch these issues early 👍. |
Thanks for the suggestion @jedcunningham. Fixed the formatting (as well as enabled pre-commit hooks). |
Awesome work, congrats on your first merged pull request! |
Congrats @bradarndt on your first commit! Thanks for contributing 🍺 |
Thanks! I appreciate the guiding hand along the way! |
This PR changes the existing behavior of "extra links" added to operators such that any relative links open in place rather than in a new window/tab. This is consistent with the way other links/buttons in Airflow work today. Any absolute links will continue to open in a new window/tab as they do today.
For the purposes of this change, a "relative" link is defined as any link url that does not satisfy the following regular expression:
^(?:[a-z]+:)?//
. See this stack overflow post for more in depth discussion.Note: As a first time contributor, I've followed the contributing documentation as much as possible. I could not find any existing tests to update for the www js code, so given the simplicity of this change, I did not add any tests, however, I did run all WWW tests as well as static analysis. Please let me know if i missed something or this is otherwise unacceptable. Looking forward to feedback!