8000 Adds conditional logic for the retrieval of credentials by mariam21arauj · Pull Request #72 · adobe/aio-cli-lib-console · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Adds conditional logic for the retrieval of credentials #72

< 8000 div class="d-flex flex-order-2 flex-md-order-1 mx-2">
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
Merged
merged 5 commits into from
Aug 29, 2023
Merged

Adds conditional logic for the retrieval of credentials #72

merged 5 commits into from
Aug 29, 2023

Conversation

mariam21arauj
Copy link
Contributor

The proposed changes will allow for the successful retrieval of either OAuth or JWT credentials through composition, so that App Builder template developers don't have the need to hardcode in the template which credential to retrieve.

Description

The new changes add conditional logic to look for OAuth credentials, and if such credentials are undefined, it will look for the JWT ones. This change will also make room for future auth providers to be added into the conditional.

Related Issue

adobe/commerce-events-ext-tpl#6

Motivation and Context

This update will benefit App Builder template developers by removing the requirement to hard code credentials in each template, whether existing or new. This change significantly reduces the need for hardcoded information in templates that support multiple authentication protocols, as well as the possibility of new auth providers to be added in the future.

How Has This Been Tested?

These changes were tested specifically with _commerce-events-ext-tpl_ commerce template for App Builder. With the proposed additional code, apps were successfully initialized and deployed for projects using both JWT and OAuth credentials, including half-way the migration process from JWT to OAuth.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@codecov
Copy link
codecov bot commented Aug 23, 2023 •
edited
Loading

Codecov Report

Merging #72 (d5eb6ce) into master (30f34d3) will not change coverage.
The diff coverage is 100.00%.

❗ Current head d5eb6ce differs from pull request most recent head e55274c. Consider uploading reports for the commit e55274c to get more accurate results

@@            Coverage Diff            @@
##            master       #72   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines          530       536    +6     
  Branches        90        92    +2     
=========================================
+ Hits           530       536    +6
Files Changed Coverage Δ
lib/index.js 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@shazron shazron added the enhancement New feature or request label Aug 24, 2023
@MichaelGoberling MichaelGoberling self-requested a review August 25, 2023 16:16
@MichaelGoberling
Copy link
Contributor
MichaelGoberling commented Aug 25, 2023 •
edited
Loading

@mariam21arauj

Nice, this would be a good addition to the lib, making it easier for template implementers to get credentials. Couple things to think about:

  • Do we want to have the CLI output that it's looking for OAuth S2S / JWT / Future credentials? Right now this function will do that because it calls the functions to get the first OAuth or JWT credentials and they both use the spinner to output text to the terminal
  • It might be more performant to have this function make the getCredentials() call once and then use helpers to try to find the first OAuth / JWT / Future credential. This also addresses the first question, because we wouldn't be calling the getFirst- functions directly, and could introduce our own spinner text in this new function

cc: @shazron

@mariam21arauj
implements changes based on feedback to avoid potentially diplaying s…
e55274c 
…pinner message twice and getCredentials to be called twice
@mariam21arauj
Copy link
Contributor Author

@mariam21arauj

Nice, this would be a good addition to the lib, making it easier for template implementers to get credentials. Couple things to think about:

  • Do we want to have the CLI output that it's looking for OAuth S2S / JWT / Future credentials? Right now this function will do that because it calls the functions to get the first OAuth or JWT credentials and they both use the spinner to output text to the terminal
  • It might be more performant to have this function make the getCredentials() call once and then use helpers to try to find the first OAuth / JWT / Future credential. This also addresses the first question, because we wouldn't be calling the getFirst- functions directly, and could introduce our own spinner text in this new function

cc: @shazron

Thanks for the feedback, Michael! Just added the suggested changes. Please let me know what you think.

@MichaelGoberling MichaelGoberling merged commit 2f8de0f into adobe:master Aug 29, 2023
@shazron
Copy link
Member
shazron commented Sep 4, 2023

@MichaelGoberling can we squash everything next time? this will be hard to revert if there was an issue in production. Not that there is right now... but investigating an issue in this whole lib currently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichaelGoberling MichaelGoberling MichaelGoberling approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mariam21arauj @MichaelGoberling @shazron
0