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

feat: support secret controller #284

Merged
merged 7 commits into from Mar 8, 2021
Merged

feat: support secret controller #284

merged 7 commits into from Mar 8, 2021

Conversation

gxthrj
Copy link
Contributor

@gxthrj gxthrj commented Mar 7, 2021

Please answer these questions before submitting a pull request


@codecov-io
Copy link

codecov-io commented Mar 7, 2021

Codecov Report

Merging #284 (8fb5f0d) into master (c65cc23) will increase coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #284      +/-   ##
==========================================
+ Coverage   52.19%   52.26%   +0.06%     
==========================================
  Files          33       32       -1     
  Lines        2320     2319       -1     
==========================================
+ Hits         1211     1212       +1     
+ Misses        952      950       -2     
  Partials      157      157              
Impacted Files Coverage Δ
pkg/seven/state/solver.go 4.00% <ø> (+2.00%) ⬆️
test/e2e/e2e.go

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c65cc23...8fb5f0d. Read the comment docs.


c.workqueue.AddRateLimited(&types.Event{
Type: types.EventAdd,
Object: obj,
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to pass key as the object, because there is a time window between we put the event into queue and we process it, in such period, the object mapped by key might be changed again in theory.

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 add Key field in types.Event, keep the original meaning of Object, because we do need some object information, when data is not available or an error is reported based on the key.

Copy link
Contributor

@tokers tokers Mar 8, 2021

Choose a reason for hiding this comment

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

If data is not available, why we should still process on it? It always means the Kubernetes Control Plane is in trouble, or the networking is partitioned, or the object was deleted, in such a case, just return error and use the retry mechanism is OK.

And, I didn't see other objects fields referred.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we have verified, if Data not available, will return error.
But we also need to record some message such as resouceVersion, which is in the object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have changed obj to key.

@tokers
Copy link
Contributor

tokers commented Mar 8, 2021

@gxthrj PR's look good to me, but we should add more descriptions about ApisixTLS to document, both the typical usage and API references should be recorded, I think we can create an issue to do it.

@gxthrj gxthrj merged commit 4262138 into apache:master Mar 8, 2021
@gxthrj gxthrj deleted the kv/secret branch March 8, 2021 08:00
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.

None yet

3 participants