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

thirdweb deploy does not respect --file option when using --ci flag #4461

Open
RyanHolanda opened this issue Sep 8, 2024 · 3 comments
Open
Assignees
Labels
Bug Something isn't working as intended in a provided reproduction. CLI CLI-related changes

Comments

@RyanHolanda
Copy link

When running the thirdweb deploy command with the --ci flag and the --file option, it still gives me the link to deploy contracts that are not in the path specified by --file.

Example:

thirdweb deploy --ci --file Router

The deployment link will still include contracts that do not have "Router" in the filename.

Solution:

I did a thorough investigation of the code, and it is not filtering the contracts when the --ci option is passed:

if (options.ci) {
selectedContracts = compiledResult.contracts;
.

An easy way to solve this would be to add filtering for the contracts in case the --file option is passed, similar to how it is done in this section of the code:

const filtered = compiledResult.contracts
.filter((c) => {
if (typeof options.file === "string" && options.file.length > 0) {
return c.fileName.includes(options.file);
}
return true;
})

@jnsdls
Copy link
Member

jnsdls commented Sep 8, 2024

hey @RyanHolanda thanks for the report and thanks for looking into how it could be fixed, massively appreciate that!

normally I'd ask if you're willing to send a PR to fix this. but in this case this would likely be much more involved, our goal is to move these CLI functionalities into a new v5 based CLI, the start of which you can find here

So if you were willing to move all of this in there (which s likely going to be quite the task) - we'd be excited to accept PRs. Otherwise if that seems too daunting I completely understand, and we'll pick this up as part of the planned migration work. (It will likely take a couple of weeks until we get to it however.)

@RyanHolanda
Copy link
Author

@jnsdls Thanks so much for being open to PRs and allowing us to contribute to the development of thirdweb tools! Unfortunately, I’m pretty busy these days (and likely for the next few weeks). I’d love to dive into contributing to the new v5-based CLI when I have more time, but for now, I’ll have to hold off 🙂

@jnsdls
Copy link
Member

jnsdls commented Sep 10, 2024

@jnsdls Thanks so much for being open to PRs and allowing us to contribute to the development of thirdweb tools! Unfortunately, I’m pretty busy these days (and likely for the next few weeks). I’d love to dive into contributing to the new v5-based CLI when I have more time, but for now, I’ll have to hold off 🙂

totally understandable, we'll pick this bug up as a part of the CLI migration in the near future!

@jnsdls jnsdls added Bug Something isn't working as intended in a provided reproduction. CLI CLI-related changes labels Sep 10, 2024
@jnsdls jnsdls self-assigned this Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working as intended in a provided reproduction. CLI CLI-related changes
Projects
None yet
Development

No branches or pull requests

2 participants