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

feat: ACNA-1602 - add logic for --no-action-code and add options object as a param #98

Closed

Conversation

arjuncooliitr
Copy link
Contributor

@arjuncooliitr arjuncooliitr commented May 26, 2022

Description

To enable preventing action code deployment using --no-action-code flag :

  • Added options objects as a param to relevant methods
  • Conditional logic to prevent adding action code on method createActionObject

Dependents

Motivation and Context

How Has This Been Tested?

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.

@arjuncooliitr arjuncooliitr requested review from shazron, moritzraho and purplecabbage and removed request for shazron May 26, 2022 12:58
Copy link
Member

@moritzraho moritzraho left a comment

Choose a reason for hiding this comment

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

overall lgtm, just one nitpick + needs testing

src/deploy-actions.js Outdated Show resolved Hide resolved
@moritzraho moritzraho self-requested a review May 30, 2022 09:10
@shazron shazron changed the title ACNA-1602: Added options object as a param feat: add options object as a param to deployActions May 30, 2022
@shazron shazron changed the title feat: add options object as a param to deployActions feat: ACNA-1602 - add options object as a param to deployActions May 30, 2022
src/deploy-actions.js Outdated Show resolved Hide resolved
@arjuncooliitr
Copy link
Contributor Author

arjuncooliitr commented Jun 7, 2022

Unit Tests (UTs) were failing with mainly 2 types of errors:

  1. UTs for func createActionObject failing with -actionCode is undefined property: I added default to options param -options = { actionCode: true } , which resolved these errors. Also, it kind of made sense to me since, by default we want to include action code unless we unset the flag.
  2. UTs for deployActions : They should now expect an additional parameter options, so modified the UTs accordingly.

All the units tests are now passing although the coverage is still not 100%. Looking to add new UTs for that.
Please review these changes @shazron

src/deploy-actions.js Outdated Show resolved Hide resolved
src/deploy-actions.js Outdated Show resolved Hide resolved
src/utils.js Outdated Show resolved Hide resolved
src/utils.js Outdated Show resolved Hide resolved
@arjuncooliitr
Copy link
Contributor Author

arjuncooliitr commented Jun 10, 2022

Not able to get 100% branch coverage for deploy-action.js. Currently its 99.85%.

Line 148 - async function deployWsk (scriptConfig, manifestContent, logFunc, filterEntities, options = { actionCode: true }) {} is missing coverage but not sure why. It is not a conditional statement so what branches are being missed? Need a bit help here @shazron @moritzraho

image

@arjuncooliitr
Copy link
Contributor Author

arjuncooliitr commented Jun 10, 2022

Tried after removing default value for options param, now getting 100% branch coverage
async function deployWsk (scriptConfig, manifestContent, logFunc, filterEntities, options) {}
Trying understand this behaviour

image

@codecov
Copy link

codecov bot commented Jun 10, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (30c96f5) 100.00% compared to head (a30eaff) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##            master       #98   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           11        11           
  Lines         1300      1311   +11     
  Branches       345       351    +6     
=========================================
+ Hits          1300      1311   +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@arjuncooliitr arjuncooliitr changed the title feat: ACNA-1602 - add options object as a param to deployActions feat: ACNA-1602 - add logic for --no-action-code and add options object as a param Jun 10, 2022
@shazron
Copy link
Member

shazron commented Jun 10, 2022

@arjuncooliitr after running the test locally, check the coverage folder on your repo. There should be an index.html file that you can load that will give you detailed coverage

src/deploy-actions.js Outdated Show resolved Hide resolved
@moritzraho moritzraho self-requested a review June 14, 2022 14:51
src/utils.js Outdated Show resolved Hide resolved
src/utils.js Outdated Show resolved Hide resolved
@arjuncooliitr arjuncooliitr marked this pull request as ready for review June 15, 2022 07:10
@arjuncooliitr arjuncooliitr requested a review from moritzraho June 16, 2022 09:38
Copy link
Member

@moritzraho moritzraho left a comment

Choose a reason for hiding this comment

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

lgtm thanks !

@shazron shazron self-assigned this Jan 30, 2023
@shazron
Copy link
Member

shazron commented Jan 30, 2023

Long overdue - if no more comments I will merge at the end of the week.

@shazron
Copy link
Member

shazron commented Jan 30, 2023

Need to resolve concerns in adobe/aio-cli-plugin-app#537 first.

@purplecabbage
Copy link
Member

This is being closed in favor of updated documentation. This pull request was well implemented, however the convenience was deemed to potentially lead to developers overwriting actions as the command syntax was awkward.
The command aio runtime action update --param is expected to be used instead of surfacing the command as an additional flag on aio app deploy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants