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

Add CORS headers to index URL used in URL check. #268

Merged
merged 4 commits into from
Jun 21, 2024
Merged

Conversation

myxie
Copy link
Collaborator

@myxie myxie commented Jun 18, 2024

Issue

We have had problems with Chrome and Opera regarding the URL check that occurs when we use the deploy button, which appear to be related to CORS errors being reported:

image

This is likely caused by the check getting the HEAD of the dim.html page, which is served with a bottle template response that does not have CORS headers added (as opposed to all the URL end points attached to the Bottle app).

Solution

I have used our existing @daliuge_aware decorator to ensure that we add CORS headers to the template response when doing the check.

Summary by Sourcery

This pull request addresses CORS errors encountered in Chrome and Opera by adding CORS headers to the 'dim.html' page response using the existing '@daliuge_aware' decorator.

  • Bug Fixes:
    • Added CORS headers to the 'dim.html' page response to resolve CORS errors in Chrome and Opera during URL checks.

Copy link
Contributor

sourcery-ai bot commented Jun 18, 2024

Reviewer's Guide by Sourcery

This pull request addresses CORS issues in Chrome and Opera by adding CORS headers to the visualizeDIM method in the daliuge-engine/dlg/manager/rest.py file. This is achieved by applying the existing @daliuge_aware decorator to the method, ensuring that the template response includes the necessary CORS headers.

File-Level Changes

Files Changes
daliuge-engine/dlg/manager/rest.py Added CORS headers to the visualizeDIM method by using the @daliuge_aware decorator.

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @myxie - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 2 issues found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

daliuge-engine/dlg/manager/rest.py Show resolved Hide resolved
def visualizeDIM(self):
"""
Note: this is marked as 'daliuge_aware' in order to get the CORS headers.
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Clarify the purpose of the note in the docstring.

The note in the docstring is useful, but it might be clearer if it specifies why CORS headers are necessary for this method. For example, is it because this method is expected to be called from a web client?

Suggested change
Note: this is marked as 'daliuge_aware' in order to get the CORS headers.
Note: this is marked as 'daliuge_aware' in order to get the CORS headers.
This is necessary because this method is expected to be called from a web client.

@myxie
Copy link
Collaborator Author

myxie commented Jun 19, 2024

I am going to merge this as the failing tests are due to the recent numpy version update. This should be resolved by removing plasma from the code base LIU-390 which will follow this PR.

@coveralls
Copy link

Coverage Status

coverage: 79.479% (+0.001%) from 79.478%
when pulling eba5bd3 on fix_cors_issue
into a055a77 on master.

@myxie myxie merged commit d2262f4 into master Jun 21, 2024
21 checks passed
awicenec pushed a commit that referenced this pull request Oct 10, 2024
Add CORS headers to index URL used in URL check.
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.

2 participants