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

Make 3scale batcher policy compatible with OIDC #956

Merged
merged 5 commits into from
Nov 23, 2018
Merged

Make 3scale batcher policy compatible with OIDC #956

merged 5 commits into from
Nov 23, 2018

Conversation

kevprice83
Copy link
Member

@kevprice83 kevprice83 commented Nov 19, 2018 •
edited
Loading

adds ability to report app_id only. Fixes THREESCALE-1515

@kevprice83 kevprice83 requested a review from a team as a code owner November 19, 2018 12:56
@kevprice83 kevprice83 requested a review from davidor November 19, 2018 12:56
@kevprice83
Copy link
Member Author

@3scale/apicast-core still need to write tests for this...

@kevprice83
Copy link
Member Author

@davidor I've pushed the integration test which at the moment is broken. Will need some help fixing that. Unit tests are passing fine.

@kevprice83
Copy link
Member Author

@mikz @davidor I'm a bit stuck now trying to get the integration test working. Any help would be appreciated.

@mayorova
Copy link
Contributor

mayorova commented Nov 21, 2018 •
edited
Loading

@kevprice83 I've changed the test, now all tests pass (I didn't check if the test would fail before your code change though).
Travis fails because of the docs - there are broken links. I've PR'ed the fix for master: #958

Once it's merged, I think it would be good to rebase this PR on top of master, and also use master as base, because I am not sure it will be included in 3.4, so 3.4-stable may be confusing.

@davidor davidor changed the base branch from 3.4-stable to master November 21, 2018 09:22
@davidor
Copy link
Contributor

davidor commented Nov 21, 2018

I've changed the base branch to master.
We've already released an RC version for 3.4.0. We are not going to release a RC-2 unless we find an important bug, so everything that we merge from now will be in the next version.

@kevprice83 @mayorova
adds integration test for an oidc configuration
fb188a7 
removes conflicting blocks and apicast blackbox

removes typo

Fix integration tests for batcher with OIDC

Fix the batcher/oidc integration test
davidor
davidor reviewed Nov 23, 2018
View reviewed changes
t/apicast-policy-3scale-batcher.t
usage_match = v == '2'
elseif k == 'transactions[0][app_id]' then
app_id_match = v == 'appid'
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also make sure that k is never transactions[0][app_key] ? Just to be sure that we are not accidentally sending it also.

@kevprice83 kevprice83 changed the title [WIP] - Make 3scale batcher policy compatible with OIDC Make 3scale batcher policy compatible with OIDC Nov 23, 2018
davidor
davidor approved these changes Nov 23, 2018
View reviewed changes
Copy link
Contributor

@davidor davidor left a comment

Choose a reason for hiding this comment

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

Thanks @kevprice83 and @mayorova 🏅 !

@davidor davidor merged commit 4e9a004 into master Nov 23, 2018
@davidor davidor deleted the fix-batcher-policy branch November 23, 2018 17:13
@davidor davidor mentioned this pull request Nov 23, 2018
Merged
@y-tabata y-tabata mentioned this pull request Dec 3, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidor davidor davidor approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@kevprice83 @mayorova @davidor