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 Dockerfile #173

Open
wants to merge 4 commits into
base: update-P21
Choose a base branch
from
Open

Conversation

danielcdz
Copy link

Issues related

Work done

  • Update Dockerfile accordingly to the main README instrucions
  • Update README
  • Add cargo install_soroban command into .cargo/config.toml

@danielcdz
Copy link
Author

Hello @Julian-dev28! Do you have any feedback?

@@ -1,7 +1,7 @@
# paths = ["/path/to/override"] # path dependency overrides

[alias] # command aliases
install_stellar = "install --locked stellar-cli --version 21.0.0 --force --root ./target stellar-cli --debug"
install_soroban = "install --git https://github.com/AhaLabs/soroban-tools --rev c7fb7e08ba8efa9828d9df863d991558f269e35b --root ./target soroban-cli --debug"
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason behind this change?

Copy link
Author

Choose a reason for hiding this comment

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

I though it was part of the dependencies needed for an update, if not I can rollback that change and update the Dockerfile accorndingly 🫡

Copy link
Member

@leighmcculloch leighmcculloch Aug 16, 2024

Choose a reason for hiding this comment

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

The example shouldn't have any special logic for installing the CLI and definitely shouldn't be installing an arbitrary commit from another repo.

Fwiw that commit sha is not currently present on any branch or tag of the stellar/stellar-cli or AhaLabs/soroban-tools repos, meaning that the commit could be from any fork and may contain history and code from anywhere.

Imo we should direct folks to the getting started help where several options for installing the CLI exist.

Why: For some folks installing the CLI from source is impossible or at least a high barrier due to resource limitations, or dependencies they don't have installed. The main install instructions for the CLI include ways to install binaries without from source, and folks should use them where possible.

So the improvement should be imo to remove this bespoke install_stellar command and ask users to install the stellar-cli themselves.

@Julian-dev28
Copy link
Contributor

Hey @danielcdz
Thanks for your submission!

I was hoping this bounty would resolve the issues encountered during the local deployment workflow, but it looks like there are still some challenges to address. Specifically, there might be an issue with the quickstart.sh file concerning the Stellar Quickstart SHA version.

It would be helpful if @leighmcculloch could weigh in on the correct SHA to use.

With this fixed, users should be able to run ./initialize.sh standalone and successfully deploy the contracts to the standalone network. Unfortunately, I encountered the following error when attempting to deploy the contracts:

Error: Networking or low-level protocol error: Server returned an error status code: 404

I hope this feedback provides some useful guidance. Looking forward to your next iteration!

@danielcdz
Copy link
Author

Hey @danielcdz Thanks for your submission!

I was hoping this bounty would resolve the issues encountered during the local deployment workflow, but it looks like there are still some challenges to address. Specifically, there might be an issue with the quickstart.sh file concerning the Stellar Quickstart SHA version.

It would be helpful if @leighmcculloch could weigh in on the correct SHA to use.

With this fixed, users should be able to run ./initialize.sh standalone and successfully deploy the contracts to the standalone network. Unfortunately, I encountered the following error when attempting to deploy the contracts:

Error: Networking or low-level protocol error: Server returned an error status code: 404

I hope this feedback provides some useful guidance. Looking forward to your next iteration!

Got it, I'll wait for @leighmcculloch for the SHA to use, also @Julian-dev28 I had a question, In the description of the issue I'm trying to solve you mentioned changes and updates in the Dockerfile and docker-compose, but you are mentioning the quickstart.sh and initialize.sh, the changes in the Dockerfile affect these sh files? I just want to know If I'm following the right path

@leighmcculloch
Copy link
Member

leighmcculloch commented Aug 16, 2024

@Julian-dev28 I commented on the thread about the SHA, see above at #173 (comment).

Regarding quickstart, the stellar-cli now embeds the quickstart, so it would be ideal if this example uses quickstart, that it is used via the stellar-cli. That might mean there is no docker-compose needed, and instead the README can say to first run stellar network contrainer start local and to then run the dapp.

I understand it may be intuitive to say that an approach like that won't be automated enough, but an approach like that is still relatively simple, and it educates a user on how to use the stellar-cli to test against a live network and teaches them how to use the foundational tools. Where-as scripts that try and hide complexity and reduce it all down to a one line command result instead in teaching a developer about those scripts and that approach.

@Julian-dev28
Copy link
Contributor

Thanks for the follow up @leighmcculloch!

@Julian-dev28
Copy link
Contributor

@danielcdz Keep up the good work. I look forward to reviewing this by next week!

@danielcdz
Copy link
Author

@Julian-dev28 @leighmcculloch I removed the install stellar command from the Dockerfile and the config.toml file, I think I'm missing the update in README.md with the suggestion @leighmcculloch made here, but I'm not sure where to add that, any suggestions?

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

Successfully merging this pull request may close these issues.

3 participants