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

Refactor http headers #2666

Merged
merged 3 commits into from
Oct 22, 2021
Merged

Refactor http headers #2666

merged 3 commits into from
Oct 22, 2021

Conversation

C0rby
Copy link
Contributor

@C0rby C0rby commented Oct 21, 2021

Description

I updated some http headers and refactored the CORS middleware. The CORS options are now configurable.
Also I removed the CORS middleware, where it isn't necessary.

Related Issue

Motivation and Context

These changes clean up some technical debt.

Types of changes

  • New feature (non-breaking change which adds functionality)
  • Technical debt (fixes technical debt, doesn't introduce it)

Checklist:

  • Code changes

@C0rby C0rby self-assigned this Oct 21, 2021
@update-docs
Copy link

update-docs bot commented Oct 21, 2021

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.

@C0rby
Copy link
Contributor Author

C0rby commented Oct 21, 2021

Ugh... the sonarcloud analysis failed because of the duplication of the new code. But this is just boilerplate code in the flagset and server commands.... Maybe we should exclude those files from the analysis?

Anyways...this PR is still good to go.

@C0rby C0rby requested review from refs, wkloucek and kobergj October 21, 2021 15:22
@C0rby C0rby marked this pull request as ready for review October 21, 2021 15:27
@C0rby
Copy link
Contributor Author

C0rby commented Oct 21, 2021

@wkloucek, any idea why this is happening?

4 errors occurred:
--> linux/386 error: exit status 1
Stderr: ../web/web.go:7:12: pattern assets/*: no matching files found

--> linux/amd64 error: exit status 1
Stderr: ../web/web.go:7:12: pattern assets/*: no matching files found

--> linux/arm64 error: exit status 1
Stderr: ../web/web.go:7:12: pattern assets/*: no matching files found

--> linux/arm error: exit status 1
Stderr: ../web/web.go:7:12: pattern assets/*: no matching files found

make: *** [../.make/release.mk:19: release-linux-docker] Error 1
make: Leaving directory '/drone/src/ocis'

@kobergj
Copy link
Collaborator

kobergj commented Oct 21, 2021

It is probably related to #2631. But it was green back then so something must have changed.

@wkloucek
Copy link
Contributor

@wkloucek, any idea why this is happening?

4 errors occurred:
--> linux/386 error: exit status 1
Stderr: ../web/web.go:7:12: pattern assets/*: no matching files found

--> linux/amd64 error: exit status 1
Stderr: ../web/web.go:7:12: pattern assets/*: no matching files found

--> linux/arm64 error: exit status 1
Stderr: ../web/web.go:7:12: pattern assets/*: no matching files found

--> linux/arm error: exit status 1
Stderr: ../web/web.go:7:12: pattern assets/*: no matching files found

make: *** [../.make/release.mk:19: release-linux-docker] Error 1
make: Leaving directory '/drone/src/ocis'

It can't find the assets... But they should be there because they have been generated... Looks flaky

refs
refs approved these changes Oct 22, 2021
View reviewed changes
Copy link
Member

@refs refs left a comment

Choose a reason for hiding this comment

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

LGTM. Just the question about if we really need CORS for the debug servers

ocis-pkg/service/debug/service.go
@@ -51,7 +52,12 @@ func NewService(opts ...Option) *http.Server {
chimiddleware.RealIP,
chimiddleware.RequestID,
middleware.NoCache,
middleware.Cors,
middleware.Cors(
Copy link
Member

Choose a reason for hiding this comment

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

do we actually need CORS for the debug service?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was asking that myself. I don't know.
How are the debug servers supposed to be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the debug service had the CORS middleware before I'll just merge this PR and if we want to remove it we can do it in another PR.

@C0rby C0rby changed the title [full-ci] Refactor http headers Refactor http headers Oct 22, 2021
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

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

0.0% 0.0% Coverage
55.8% 55.8% Duplication

@C0rby C0rby merged commit d71907f into master Oct 22, 2021
@delete-merged-branch delete-merged-branch bot deleted the http-header branch October 22, 2021 12:35
ownclouders pushed a commit that referenced this pull request Oct 22, 2021
commit d71907f
0483555 
Merge: 4b43c5f 9ecc065
Author: David Christofas <[email protected]>
Date:   Fri Oct 22 14:35:45 2021 +0200

    Merge pull request #2666 from owncloud/http-header

    Refactor http headers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refs refs refs approved these changes

@wkloucek wkloucek Awaiting requested review from wkloucek

@kobergj kobergj Awaiting requested review from kobergj

@kulmann kulmann Awaiting requested review from kulmann

@pascalwengerter pascalwengerter Awaiting requested review from pascalwengerter

@lookacat lookacat Awaiting requested review from lookacat

Assignees

@C0rby C0rby

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Config for Cors headers seems lost during the config refactoring
4 participants
@C0rby @kobergj @wkloucek @refs