-
Notifications
You must be signed in to change notification settings - Fork 352
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
Use ACL server for CI #8155
Use ACL server for CI #8155
Conversation
e1029e1
to
d11938d
Compare
83ebe97
to
ba62199
Compare
ba62199
to
a788065
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few comments that IMO help clarifying some confusing terms. Other than that LGTM
@@ -1361,7 +1421,6 @@ jobs: | |||
ESTI_AWS_ACCESS_KEY_ID: ${{ secrets.ESTI_AWS_ACCESS_KEY_ID }} | |||
ESTI_AWS_SECRET_ACCESS_KEY: ${{ secrets.ESTI_AWS_SECRET_ACCESS_KEY }} | |||
ESTI_VERSION: ${{ needs.deploy-image.outputs.tag }} | |||
ESTI_AUTH_BASIC: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it is not longer needed. It was used during development
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename this file to match the others? docker-compose-dynamodb
, docker-compose-external-db
& docker-compose
is quite confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest a name
return getRBACState(t, ctx) == "none" | ||
} | ||
|
||
func isAdvancedAuth(t testing.TB, ctx context.Context) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ACL server configuration returns true
for this? If so, it's quite confusing as ACL isn't the advanced auth that we got (it's not RBAC)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it does not. It return false
Closes #8128
Change Description
Use ACL server for CI instead of fluffy