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(publish): Support for custom VCL logic #893

Merged
merged 14 commits into from
May 20, 2019
Merged

Conversation

kptdobe
Copy link
Contributor

@kptdobe kptdobe commented May 15, 2019

PR for #812

@codecov
Copy link

codecov bot commented May 15, 2019

Codecov Report

Merging #893 into master will decrease coverage by 6.17%.
The diff coverage is 88.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #893      +/-   ##
==========================================
- Coverage   91.75%   85.58%   -6.18%     
==========================================
  Files          43       43              
  Lines        1796     1804       +8     
==========================================
- Hits         1648     1544     -104     
- Misses        148      260     +112
Impacted Files Coverage Δ
src/publish.js 30% <ø> (-60%) ⬇️
src/remotepublish.cmd.js 84.1% <88.23%> (-15.9%) ⬇️
src/clean.cmd.js 20.83% <0%> (-79.17%) ⬇️
src/up.js 27.77% <0%> (-55.56%) ⬇️
src/yargs-github.js 50% <0%> (-50%) ⬇️
src/git-utils.js 62.65% <0%> (-37.35%) ⬇️
src/package.cmd.js 88.11% <0%> (-1.99%) ⬇️
src/hack.js 100% <0%> (ø) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aedbd85...5dbad80. Read the comment docs.

@codecov
Copy link

codecov bot commented May 15, 2019

Codecov Report

Merging #893 into master will increase coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #893      +/-   ##
==========================================
+ Coverage   91.75%   91.84%   +0.08%     
==========================================
  Files          43       43              
  Lines        1796     1814      +18     
==========================================
+ Hits         1648     1666      +18     
  Misses        148      148
Impacted Files Coverage Δ
src/publish.js 90% <ø> (ø) ⬆️
src/remotepublish.cmd.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86f1b62...41c380d. Read the comment docs.

Copy link
Contributor

@tripodsan tripodsan left a comment

Choose a reason for hiding this comment

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

  • add tests for passing the vlc arguments to test/testPublishCli.js

@@ -155,6 +158,30 @@ class RemotePublishCommand extends AbstractCommand {
return this;
}

withCustomVCL(value) {
if (value && value.length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

from an API perspective, I think this should also allow adding single VLCs and update the internal this._vcl.
otherwise, rename it to withCustomVCLs()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the parameter custom-vcl (list of file paths) has to be transformed into an object (name:contentoffile), I initially thought we could have the 2 methods to have the 2 ways of providing the data: either the list of files or the object directly. But you are right, this is confusing and introduce useless features. I removed the second method.

return this;
}

withVCL(value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

  • no jsdoc
  • what's the purpose of this in contrast to withCustomVCL ? do you really need both?
  • needs to be an object, so name it withVCLs

@@ -155,6 +158,30 @@ class RemotePublishCommand extends AbstractCommand {
return this;
}

withCustomVCL(value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

no jsdoc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah ah. I adopted the jsdoc pattern of that file :) but you are right, that's not a reason, we need to start somewhere.

await remote.run();

assert.deepEqual(vcl, {
extensions: fs.readFileSync(e).toString(),
Copy link
Contributor

Choose a reason for hiding this comment

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

async, please

test/testRemotePublishCmd.customvcl.js Outdated Show resolved Hide resolved

// eslint-disable-next-line no-underscore-dangle
assert.deepEqual(remote._vcl, {
extensions: fs.readFileSync(e).toString(),
Copy link
Contributor

Choose a reason for hiding this comment

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

async please

Co-Authored-By: Tobias Bocanegra <tripodsan@users.noreply.github.com>
@ghost
Copy link

ghost commented May 16, 2019

There were the following issues with this Pull Request

  • Commit: d0f7367
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

1 similar comment
@ghost
Copy link

ghost commented May 16, 2019

There were the following issues with this Pull Request

  • Commit: d0f7367
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@ghost
Copy link

ghost commented May 16, 2019

There were the following issues with this Pull Request

  • Commit: d0f7367
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@ghost
Copy link

ghost commented May 16, 2019

There were the following issues with this Pull Request

  • Commit: d0f7367
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@ghost
Copy link

ghost commented May 16, 2019

There were the following issues with this Pull Request

  • Commit: d0f7367
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@ghost
Copy link

ghost commented May 16, 2019

There were the following issues with this Pull Request

  • Commit: d0f7367
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@ghost
Copy link

ghost commented May 16, 2019

There were the following issues with this Pull Request

  • Commit: d0f7367
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@ghost
Copy link

ghost commented May 16, 2019

There were the following issues with this Pull Request

  • Commit: d0f7367
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@ghost
Copy link

ghost commented May 16, 2019

There were the following issues with this Pull Request

  • Commit: d0f7367
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@ghost
Copy link

ghost commented May 16, 2019

There were the following issues with this Pull Request

  • Commit: d0f7367
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@kptdobe
Copy link
Contributor Author

kptdobe commented May 16, 2019

Yeah... managed to bring the code coverage back to positive!

@ghost
Copy link

ghost commented May 20, 2019

There were the following issues with this Pull Request

  • Commit: d0f7367
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@kptdobe kptdobe merged commit d773f2a into master May 20, 2019
@kptdobe kptdobe deleted the support-custom-vcl branch May 20, 2019 08:53
trieloff pushed a commit that referenced this pull request May 20, 2019
# [3.1.0](v3.0.0...v3.1.0) (2019-05-20)

### Bug Fixes

* **package:** update snyk to version 1.165.1 ([79e12f7](79e12f7))

### Features

* **publish:** Support for custom VCL logic ([#893](#893)) ([d773f2a](d773f2a))
@adobe-bot
Copy link
Collaborator

🎉 This PR is included in version 3.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants