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

update major for aio-cli-lib-app-config #682

Closed
wants to merge 10 commits into from
Closed

Conversation

moritzraho
Copy link
Member

@moritzraho moritzraho commented Jun 7, 2023

Description

TO DISCUSS: potential breaking change because now every command that loads the configuration will undergo schema validation (via load in aio-cli-lib-app-config) and we may have some corner cases that were working without being valid

Depends on a release for adobe/aio-cli-lib-app-config#21 and adobe/aio-cli-lib-app-config#22

  • async config
  • no validation here, use aio-cli-lib-app-config instead.

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.

@moritzraho moritzraho marked this pull request as ready for review June 8, 2023 12:56
@moritzraho
Copy link
Member Author

Note tests are failing here but passing with local linking of aio-cli-lib-app-config

Copy link
Member

@shazron shazron left a comment

Choose a reason for hiding this comment

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

LGTM for code review - I haven't had a chance to test it by running the command yet, will do so next week

@moritzraho
Copy link
Member Author

do not merge yet.
pre-release at @adobe/aio-cli-plugin-app@next
needs release of aio-cli-lib-app-config and updated package.json

@codecov
Copy link

codecov bot commented Jul 17, 2023

Codecov Report

Merging #682 (fa423c2) into master (a7189d4) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #682   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           57        57           
  Lines         3152      3156    +4     
  Branches       589       589           
=========================================
+ Hits          3152      3156    +4     
Files Changed Coverage Δ
src/BaseCommand.js 100.00% <100.00%> (ø)
src/commands/app/add/action.js 100.00% <100.00%> (ø)
src/commands/app/add/event.js 100.00% <100.00%> (ø)
src/commands/app/add/extension.js 100.00% <100.00%> (ø)
src/commands/app/add/web-assets.js 100.00% <100.00%> (ø)
src/commands/app/build.js 100.00% <100.00%> (ø)
src/commands/app/config/get/log-forwarding.js 100.00% <100.00%> (ø)
...c/commands/app/config/get/log-forwarding/errors.js 100.00% <100.00%> (ø)
src/commands/app/config/set/log-forwarding.js 100.00% <100.00%> (ø)
src/commands/app/delete/action.js 100.00% <100.00%> (ø)
... and 13 more

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

@moritzraho
Copy link
Member Author

postponed for now - needs a --no-config-validation flag

@moritzraho
Copy link
Member Author

closing this for now as it is outdated

@purplecabbage
Copy link
Member

instead of forcing a strict validation on every cli command, it might make more sense to just add a validate command and suggest developers run it if there is is an issue with config

@moritzraho
Copy link
Member Author

For now let's just disable validation to move forward, see adobe/aio-cli-lib-app-config#32

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.

3 participants