Skip to content

fix: Use correct binary path on Windows#231

Merged
kodiakhq[bot] merged 3 commits intocloudquery:mainfrom
erezrokah:fix/binary_path_on_windows
Oct 3, 2022
Merged

fix: Use correct binary path on Windows#231
kodiakhq[bot] merged 3 commits intocloudquery:mainfrom
erezrokah:fix/binary_path_on_windows

Conversation

@erezrokah
Copy link
Copy Markdown
Member

Summary

Based on go source code binaries on Windows must end in exe, cmd, etc. See https://github.com/golang/go/blob/f6d844510d5f1e3b3098eba255d9b633d45eac3b/src/os/exec/lp_windows.go#L39
⬆️ Is the code that looks up the binary when you use the exec package


Use the following steps to ensure your PR is ready to be reviewed

  • Read the contribution guidelines 🧑‍🎓
  • Run go fmt to format your code 🖊
  • Lint your changes via golangci-lint run 🚨 (install golangci-lint here)
  • Update or add tests 🧪
  • Ensure the status checks below are successful ✅

Copy link
Copy Markdown
Contributor

@yevgenypats yevgenypats left a comment

Choose a reason for hiding this comment

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

I think on the save side let's make it consistent without suffix

}
org, name := pathSplit[0], pathSplit[1]
localPath := filepath.Join(c.directory, "plugins", string(PluginTypeDestination), org, name, version, "plugin")
localPath = withBinarySuffix(localPath)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think we should save it with .exe (I get the goreleaser part but no need to do it on the save part as it will work without the exe)

Copy link
Copy Markdown
Member Author

@erezrokah erezrokah Oct 3, 2022

Choose a reason for hiding this comment

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

}
org, name := pathSplit[0], pathSplit[1]
localPath := filepath.Join(c.directory, "plugins", string(PluginTypeSource), org, name, version, "plugin")
localPath = withBinarySuffix(localPath)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this wont be needed.

if runtime.GOOS == "windows" {
pathInArchive += ".exe"
}
pathInArchive = withBinarySuffix(pathInArchive)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we just needed it here so no need to do withBinarySuffix

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We already have it here and it still fails (see failure in other comment)

@kodiakhq kodiakhq bot merged commit 0a5dc26 into cloudquery:main Oct 3, 2022
kodiakhq bot pushed a commit that referenced this pull request Oct 3, 2022
🤖 I have created a release *beep* *boop*
---


## [0.11.7](v0.11.6...v0.11.7) (2022-10-03)


### Bug Fixes

* Set default download directory to `.cq` ([#230](#230)) ([689f5ed](689f5ed))
* Use correct binary path on Windows ([#231](#231)) ([0a5dc26](0a5dc26))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
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