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.

Sign up for GitHub

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

Jump to bottom

Add tutorial 02 to CI #566

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Add tutorial 02 to CI #566

wants to merge 5 commits into from

Conversation

Trivo25
Copy link
Member

@Trivo25 Trivo25 commented Aug 17, 2023 •
edited by barriebyron
Loading

The Tutorial Runner was implemented only for Tutorial 1; this PR adds it to Tutorial 2

@Trivo25 Trivo25 requested a review from a team as a code owner August 17, 2023 15:48
@vercel
Copy link

vercel bot commented Aug 17, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs2 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 3, 2023 10:02pm

ymekuria
ymekuria approved these changes Aug 17, 2023
View reviewed changes
Copy link
Contributor

@ymekuria ymekuria left a comment

Choose a reason for hiding this comment

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

Nice @Trivo25! This will be helpful to have. I just had a comment about upgrading the Node version here and for the other tutorials

.github/workflows/test-tutorials.yml Outdated
- uses: actions/setup-node@v3
with:
# In case of version change, update README.md accordingly
node-version: 16
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably should update the node version here and with the other tutorials in CI because node 16 has reached end of life.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, @shimkiv has landed o1-labs/zkapp-cli#479

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 right update here?

barriebyron
barriebyron approved these changes Sep 7, 2023
View reviewed changes
Copy link
Contributor

@barriebyron barriebyron left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM I fixed conflicts in fe89c95

(due to this work using the previous name that we will no longer name)

@barriebyron
Copy link
Contributor

@Trivo25 same comment as #577 (comment)

I lost track of the status of this PR... were we waiting to merge it until the dependency on line numbers in the tutorial runner is removed? If we are waiting, wdyt we can move the PR to draft?

@Trivo25 Trivo25 marked this pull request as draft November 22, 2023 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@barriebyron barriebyron barriebyron approved these changes

@ymekuria ymekuria ymekuria approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Trivo25 @barriebyron @ymekuria