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

fix: resolve missing output when using --json #4759

Merged
merged 3 commits into from Jul 11, 2022
Merged

fix: resolve missing output when using --json #4759

merged 3 commits into from Jul 11, 2022

Conversation

dariodevito
Copy link
Contributor

@dariodevito dariodevito commented Jul 1, 2022

Summary

Fixes #4758

Adds support for --json when using with netlify deploy --trigger.


For us to review and ship your PR efficiently, please perform the following steps:

  • Open a bug/issue before writing your code πŸ§‘β€πŸ’». This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire πŸ”₯ (e.g. incident related), you can skip this step.
  • Read the contribution guidelines πŸ“–. This ensures your code follows our style guide and
    passes our tests.
  • [] Update or add tests (if any source code was changed or added) πŸ§ͺ
  • Update or add documentation (if features were changed or added) πŸ“
  • Make sure the status checks below are successful βœ…

Image from iOS


Just to call out, I had intended to add an integration test in ./tests/integration/210.command.deploy.test.js along the lines of below. However, because the test site isn't built from/linked to a git codebase, the --trigger option (which calls createSiteBuild) returns a 404 (doesn't have a codebase to build from).

I think it would make sense for a Netlify developer to specify the repo this should be linked to (and by extension add tests for --trigger. (I'm guessing that is why it hasn't been covered before now)

  test.serial('should return valid json when both --trigger and --json are passed', async (t) => {
    await withSiteBuilder('site-with-public-folder', async (builder) => {
      const content = '<h1>βŠ‚β—‰β€Ώβ—‰γ€</h1>'
      builder
        .withContentFile({
          path: 'public/index.html',
          content,
        })
        .withNetlifyToml({
          config: {
            build: { publish: 'public' },
          },
        })

      await builder.buildAsync()

      const output = await callCli(['deploy', '--trigger', '--json'], {
        cwd: builder.directory,
        env: { NETLIFY_SITE_ID: t.context.siteId },
      })

      JSON.parse(output)
    })
  })

@danez danez added the type: bug code to address defects in shipped code label Jul 4, 2022
@danez
Copy link
Contributor

danez commented Jul 4, 2022

Thanks for creating the PR.

We will check the test case you mentioned.

Copy link
Contributor

@danez danez 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 again, I created a separate issue for the test.

@@ -553,7 +562,7 @@ const deploy = async (options, command) => {
const deployToProduction = options.prod || (options.prodIfUnlocked && !siteData.published_deploy.locked)

if (options.trigger) {
return triggerDeploy({ api, siteId, siteData })
return triggerDeploy({ api, options, siteData, siteId })
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for ordering 😘

@danez danez added the automerge Add to Kodiak auto merge queue label Jul 8, 2022
@kodiakhq kodiakhq bot merged commit 4b4b174 into netlify:main Jul 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Add to Kodiak auto merge queue type: bug code to address defects in shipped code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

deploy doesn't output when using --trigger & --json together
2 participants