Skip to content

🌱 cache: moves common HTTP handlers to a shared pkg #1947

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

Merged
merged 4 commits into from
Sep 12, 2022

Conversation

p0lyn0mial
Copy link
Contributor

Summary

It moves common HTTP handlers to a separate package.
Those handlers are used by the kcp-server and the cache-server.
Since we are going to run the cache from the kcp.
Importing will cause a cycle.
Thus common functionality is moved to a common package.

Related issue(s)

Fixes #

@p0lyn0mial p0lyn0mial requested review from stevekuznetsov and removed request for stevekuznetsov September 12, 2022 07:59
@p0lyn0mial p0lyn0mial mentioned this pull request Sep 12, 2022
37 tasks
apiHandler = WithAcceptHeader(apiHandler)
apiHandler = WithUserAgent(apiHandler)
apiHandler = util.WithAcceptHeader(apiHandler)
apiHandler = util.WithUserAgent(apiHandler)
Copy link
Member

Choose a reason for hiding this comment

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

am not convinced we want to share any of these with the cache server. Why should we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WithUserAgent wasn't required also was able to get rid of WithAcceptHeader

@sttts
Copy link
Member

sttts commented Sep 12, 2022

/hold

Wrong direction. We should not share the handler chain between kcp and the caching server. Why should we?

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 12, 2022
@@ -184,19 +78,6 @@ func WithAuditAnnotation(handler http.Handler) http.HandlerFunc {
})
}

// WithClusterAnnotation adds the cluster name into the annotation of an audit
// event. Needs initialized annotations.
func WithClusterAnnotation(handler http.Handler) http.HandlerFunc {
Copy link
Member

Choose a reason for hiding this comment

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

this needs a better name. WithAuditEventClusterAnnotation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

Those handlers are used by the kcp-server and the cache-server.
Since we are going to run the cache from the kcp.
Importing will cause a cycle. Thus common functionality is moved to a common package.
@p0lyn0mial p0lyn0mial force-pushed the cache-handlers-common branch from 3da47f1 to d7ad4b5 Compare September 12, 2022 12:06
@sttts
Copy link
Member

sttts commented Sep 12, 2022

/hold cancel
/lgtm
/approve

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Sep 12, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 12, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 12, 2022
@p0lyn0mial p0lyn0mial force-pushed the cache-handlers-common branch from d7ad4b5 to 9978144 Compare September 12, 2022 12:29
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 12, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 12, 2022

New changes are detected. LGTM label has been removed.

@p0lyn0mial p0lyn0mial added the lgtm Indicates that a PR is ready to be merged. label Sep 12, 2022
@p0lyn0mial
Copy link
Contributor Author

added lgtm label manually since I only fixed the boilterplate

@openshift-merge-robot openshift-merge-robot merged commit 37c3a47 into kcp-dev:main Sep 12, 2022
@stevekuznetsov
Copy link
Contributor

#342

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants