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

Added support for httpd auth-api service for containers. #15881

Merged
merged 3 commits into from
Sep 26, 2017

Conversation

abellotti
Copy link
Member

@abellotti abellotti commented Aug 22, 2017

  • Enhanced fetching user attributes via dbus to use the httpd auth-api service for containers.
  • Enhanced fetching user groups via dbus to use the httpd auth-api service for containers.
  • Updated rspecs

Related PRs:

@miq-bot
Copy link
Member

miq-bot commented Aug 30, 2017

This pull request is not mergeable. Please rebase and repush.

@abellotti abellotti force-pushed the ext-auth-api-service branch 3 times, most recently from 5cf8993 to a7a610f Compare August 30, 2017 21:59
@abellotti abellotti changed the title [WIP] Added support for httpd auth-api service for containers. Added support for httpd auth-api service for containers. Aug 30, 2017
@abellotti abellotti force-pushed the ext-auth-api-service branch from a7a610f to 4a8721a Compare August 30, 2017 22:03
@abellotti abellotti force-pushed the ext-auth-api-service branch 2 times, most recently from 6247173 to a708ff9 Compare August 31, 2017 00:20
@ManageIQ ManageIQ deleted a comment from miq-bot Aug 31, 2017
@abellotti abellotti force-pushed the ext-auth-api-service branch from a708ff9 to 826a38d Compare August 31, 2017 01:01
@carbonin
Copy link
Member

carbonin commented Sep 7, 2017

I have no idea what the lib/services directory is supposed to be used for. Is that the best place for this? @Fryguy ?

@Fryguy
Copy link
Member

Fryguy commented Sep 8, 2017

If it's going into services, it should be namespaced properly for autoload, so it should be Services::AuthApiService, which I don't like. I think it should just be in the lib directory directly.

@abellotti
Copy link
Member Author

With it not being in lib/services, maybe it should also be renamed to HttpdAuthApi. Thoughts ?

@Fryguy
Copy link
Member

Fryguy commented Sep 8, 2017

I'm good with HttpdAuthApi

- Enhanced fetching user attributes via dbus to use the httpd auth-api service for containers.
- Enhanced fetching user groups via dbus to use the httpd auth-api service for containers.
- Updated rspecs
- dbus dropped from the URL
- Moved logic to a common AuthApiService
- Added tests for AuthApiService
- Moved the library from lib/services/auth_api_service.rb to lib/httpd_auth_api.rb
@abellotti abellotti force-pushed the ext-auth-api-service branch from 826a38d to 0fb40b1 Compare September 9, 2017 18:53
@miq-bot
Copy link
Member

miq-bot commented Sep 9, 2017

Checked commits abellotti/manageiq@6dc35a1~...0fb40b1 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
6 files checked, 0 offenses detected
Everything looks fine. ⭐

def auth_api(request_url)
host = ENV["HTTPD_AUTH_API_SERVICE_HOST"]
port = ENV["HTTPD_AUTH_API_SERVICE_PORT"]
conn = Faraday.new(:url => "http://#{host}:#{port}") do |faraday|
Copy link
Member

Choose a reason for hiding this comment

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

Should this use the URI::HTTP.build?

@carbonin carbonin requested a review from Fryguy September 14, 2017 14:57
@carbonin carbonin assigned carbonin and unassigned gtanzillo Sep 26, 2017
@carbonin carbonin added this to the Sprint 70 Ending Oct 2, 2017 milestone Sep 26, 2017
@carbonin carbonin merged commit 91d3ef4 into ManageIQ:master Sep 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants