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.

Sign up for GitHub

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

Jump to bottom

[full-ci] Add Cache-Control to default allowedCorsHeaders #40024

Merged
merged 1 commit into from
Apr 29, 2022
Merged

[full-ci] Add Cache-Control to default allowedCorsHeaders #40024

merged 1 commit into from
Apr 29, 2022

Conversation

dschmidt
Copy link
Member

Description

I'm trying to get this PR green: owncloud/web#6494
It makes certain requests from oC Web send a Cache-Control header, which fails in a CORS setup.
As you can see, I added php occ config:system:set cors.allowed-headers --type json --value '["cache-control"]' which fixed the tests locally for me. Still waiting for the CI to finish at this moment.

Related Issue

Motivation and Context

c.f. owncloud/web#6447
"When apps (i.e Drawio) try to load a file, the browser caches the request.
If the file was modified somewhere else, this causes inconsistent results which prevent saving any changes until the cache is properly cleared.

We had observed this is the past in our text editor, hence the noCache addition to the SDK. Now we also saw it in Drawio."

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@update-docs
Copy link

update-docs bot commented Apr 27, 2022

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@ownclouders
Copy link
Contributor

ownclouders commented Apr 27, 2022 •
edited
Loading

💥 Acceptance tests pipeline webUIFfSmoke-3-2-firefox-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/35536/166

dschmidt added a commit to owncloud/web that referenced this pull request Apr 27, 2022
@dschmidt
Workaround CORS issue until owncloud/core#40024 is merged and released
caf2dab
@dschmidt dschmidt mentioned this pull request Apr 27, 2022
Merged
9 tasks
@dschmidt
Copy link
Member Author

Are those CI failures expected? Cannot make sense of them in relation to my change

pascalwengerter pushed a commit to owncloud/web that referenced this pull request Apr 27, 2022
@phil-davis @diocas @dschmidt
Do not load files from cache (#6447) (#6494)
5fbfb78 
* Do not load files from cache (#6447)

* Do not load files from cache

* changelog

* Workaround CORS issue until owncloud/core#40024 is merged and released

* Update changelog with further information on Cache-Control header

Co-authored-by: Diogo Castro <[email protected]>
Co-authored-by: Dominik Schmidt <[email protected]>
@phil-davis
Copy link
Contributor

Are those CI failures expected? Cannot make sense of them in relation to my change

https://drone.owncloud.com/owncloud/core/35490/10/8
There were 5 failures:
And all are in CorsPluginTest

That seems related.

@dschmidt
Copy link
Member Author

You're right, I did miss to see the actual failure, what I was seeing was just noise. Will fix!

@dschmidt dschmidt force-pushed the add_cache_control_to_default_allowed_cors_headers branch 2 times, most recently from 2e0d20e to 66a81da Compare April 28, 2022 07:56
@dschmidt
Copy link
Member Author

Why do the CI builds keep getting canceled?
I just see canceled pipelines but no failed ones...

@phil-davis
Copy link
Contributor

Why do the CI builds keep getting canceled? I just see canceled pipelines but no failed ones...

I noticed that. "someone" or "something" must be cancelling them. The drone timeout for the core repo is set to 3 hours at https://drone.owncloud.com/owncloud/core/settings

@dschmidt
Copy link
Member Author

I've kept restarting it, but to no avail - it keeps being canceled. No idea why. Can anyone help me?

@dschmidt dschmidt changed the title Add Cache-Control to default allowedCorsHeaders [full-ci] Add Cache-Control to default allowedCorsHeaders Apr 28, 2022
@dschmidt dschmidt force-pushed the add_cache_control_to_default_allowed_cors_headers branch from 66a81da to 9957097 Compare April 28, 2022 21:50
@dschmidt
Copy link
Member Author

Aha, at last it looks like a real error: https://drone.owncloud.com/owncloud/core/35537/100/13

@dschmidt dschmidt force-pushed the add_cache_control_to_default_allowed_cors_headers branch from 9957097 to 202a3ff Compare April 28, 2022 23:02
@dschmidt dschmidt force-pushed the add_cache_control_to_default_allowed_cors_headers branch from 202a3ff to 2bc1d98 Compare April 28, 2022 23:05
@sonarcloud
Copy link

sonarcloud bot commented Apr 28, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@dschmidt
Copy link
Member Author

Aaaw yiiiss, finally green.

@IljaN wanna review again after my force push? Feel free to merge if you think this is ok.

@phil-davis phil-davis merged commit a3be671 into master Apr 29, 2022
@delete-merged-branch delete-merged-branch bot deleted the add_cache_control_to_default_allowed_cors_headers branch April 29, 2022 03:39
@jnweiger jnweiger mentioned this pull request May 17, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phil-davis phil-davis phil-davis approved these changes

@IljaN IljaN IljaN approved these changes

@DeepDiver1975 DeepDiver1975 Awaiting requested review from DeepDiver1975

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dschmidt @ownclouders @phil-davis @IljaN