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

bump fog openstack gem to new version 1.1.1 & enable application credential authentication #272

Merged
merged 6 commits into from
May 16, 2024

Conversation

fmoehler
Copy link
Contributor

No description provided.

@fmoehler fmoehler changed the title bump fog openstack gem to new version 1.1.1 bump fog openstack gem to new version 1.1.1 & enable application credential authentication May 15, 2024
@fmoehler fmoehler marked this pull request as draft May 15, 2024 08:49
@fmoehler fmoehler marked this pull request as ready for review May 15, 2024 09:37
@@ -778,6 +782,20 @@ def validate_options
},
optional('agent') => Hash,
}

unless ((username && api_key) || (application_credential_id && application_credential_secret)) ||
(username && api_key && application_credential_id && application_credential_secret)
Copy link

Choose a reason for hiding this comment

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

What do we check with (username && api_key && application_credential_id && application_credential_secret) ?
Finally all of the properties must not be nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We wanted to check that not all of these credentials are provided.

Copy link

Choose a reason for hiding this comment

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

But then it should be

unless ((username && api_key) || (application_credential_id && application_credential_secret)) &&
          !(username && api_key && application_credential_id && application_credential_secret)

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, may be splitting this into two boolean variables make it more readable?
e.g.: using_api_key = username && api_key?

Copy link

@mvach mvach left a comment

Choose a reason for hiding this comment

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

please check above comment.

Copy link

@mvach mvach left a comment

Choose a reason for hiding this comment

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

please check above comment.

Copy link

linux-foundation-easycla bot commented May 15, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link

@mvach mvach left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@a-hassanin a-hassanin left a comment

Choose a reason for hiding this comment

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

Looks good now!

@fearoffish fearoffish force-pushed the bump-fog-openstack-gem branch from 5429e79 to f416866 Compare May 16, 2024 08:44
@mvach mvach merged commit 6609d34 into cloudfoundry:master May 16, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants