guild-operators moving pr 1743 to node 8.9.1

Description

Branched from: https://github.com/cardano-community/guild-operators/pull/1743
Updated CARDANO_NODE_VERSION to 8.9.1
updated node-deps.json to 8.9.1
updated blst to latest per release specs
Added Fork/Branch ARG to dockerfile_bin & entrypoint.sh to enable docker test/dev on forked repos
added missing jg dep in docker build based on build errors

Where should the reviewer start?

This should be merged into the original branch (node-8.9.0) and tested more extensively. I can help in that effort but this is my first contribution and my automated test env and familiarity is lacking.

Motivation and context

Reduce node release implementation timetable for guild-operators docker images.
Enable docker development from Forks
Give back and contribute to the guild-operators

Which issue it fixes?

updating this existing branch (node-8.9.0) to 8.9.1 release ref PR: (https://github.com/cardano-community/guild-operators/pull/1743)

How has this been tested?

Successfully deployed and completely synced nodes using the Docker image and Host env on mainnet, preprod, preview networks. No further functionality has been verified. This should be considered as untested and is not ready for production.

Apr 09 '24 20:04 Fuma419

Another way to handle this PR would be to merge to alpha then close https://github.com/cardano-community/guild-operators/pull/1743

Apr 09 '24 20:04 Fuma419

Separate from the review comments already left, I don't believe using node-8.9.0 as the base is correct. The tags are intended to provide a static commit that can be used when breaking changes occur within the master branch. I don't believe they get updated after that point, but I will defer to @rdlrt and @Scitz0 on that point.

Apr 10 '24 00:04 TrevorBenson

Separate from the review comments already left, I don't believe using node-8.9.0 as the base is correct. The tags are intended to provide a static commit that can be used when breaking changes occur within the master branch. I don't believe they get updated after that point, but I will defer to @rdlrt and @Scitz0 on that point.

Do you propose that the work to implement 8.9.1 should have been baselined from tag 8.1.2? Should we just discard all of the 8.9.0 effort? I reviewed the work in node-8.9.0 and found value and intended to preserve it.

fwiw, this is my first attempt at contributing to this project. I have been a user and supporter for some time.

Apr 10 '24 04:04 Fuma419

Separate from the review comments already left, I don't believe using node-8.9.0 as the base is correct. The tags are intended to provide a static commit that can be used when breaking changes occur within the master branch. I don't believe they get updated after that point, but I will defer to @rdlrt and @Scitz0 on that point.

Do you propose that the work to implement 8.9.1 should have been baselined from tag 8.1.2? Should we just discard all of the 8.9.0 effort? I reviewed the work in node-8.9.0 and found value and intended to preserve it.

Honestly, this is my first attempt at contributing to this project. I have been a user and supporter for some time. Based on my first impressions from TrevorBenson 's responses above this does not seem like a very collaborative or hospitable space. please prove me wrong

I'm not referring to the starting point of your branch. I was referring to the base branch for your PR to merge into cardano-community:node-8.9.0 instead of cardano-community:alpha.

Generally all PR's target alpha. A tag, node-8.9.0 in this case I believe is simply created prior to merging any breaking changes into master. Thus node versions may not exists as a tag in the guild-operators repository between many versions when config changes etc. do not cause a breaking change.

Or at least that's what I recall of how tags are used in this repository. If I describe this incorrectly @rdlrt or @Scitz0 would be the ones to clarify if this was changed at some point.

Apr 10 '24 15:04 TrevorBenson

node-8.9.0

I believe node-8.9.0 is a branch and not a tag: https://github.com/cardano-community/guild-operators/tree/node-8.9.0

I would expect the tag to be attached to a release commit post PR merge.

Maybe the solution is for me to just target alpha directly in this PR. Please advise.

Apr 10 '24 15:04 Fuma419

node-8.9.0

I believe node-8.9.0 is a branch and not a tag: https://github.com/cardano-community/guild-operators/tree/node-8.9.0

I would expect the tag to be attached to a release commit post PR merge.

Maybe the solution is for me to just target alpha directly in this PR. Please advise.

You are correct, I had not realized node-8.9.0 was the branch to merge in PR for #1743 which is still open. My apologies for the confusion this added.

Apr 10 '24 15:04 TrevorBenson

Normally we target alpha with all PRs. But in this case I think it's fine to target node-8.9.0 branch as this was never merged due to uncertainty around surrounding tools like dbsync and more. And we have 8.10.x around the corner that will change stuff again.

I think we will stick to 8.7.3 on alpha/master for now until we reach a somewhat stable state with various tooling being able to catch up.

With that said I appreciate the PR and having target as 8.9.0 branch is the least bad thing right now 🙂. For those advanced users that want to run the latest released version.

Apr 10 '24 16:04 Scitz0

Just to confirm the plan ahead, we would like to skip 8.9.x (given the state on preprod - the node upgrade to 8.9.x should've gone via a fork - on mainnet, largest stakeholders being exchanges who are usually very late to upgrade) altogether, hopefully 8.10.x is a bit more coordinated (has all core components available) - and is easier to merge to alpha. The PR branch #1743 would be closed and re-raised into 8.10.x updating once the releases are available.

Meanwhile, happy to continue work related to scripts targetting 8.9.0 (meant for 8.9.x) branch - but maybe docker related file changes could be better as a seperate PR? Just thinking from the perspective of doing the merge once ready (from test failures pov, which I believe is still due to G_ACCOUNT) to the target branch itself

Apr 16 '24 00:04 rdlrt

@TrevorBenson

Other contributors should review/approve for the changes to node-deps.json and cncli.sh prior to merge.

The cncli.sh changes are already on alpha (and updated on target node-8.9.0 branch), while the node-deps has node version bump but since we're not going ahead with any of the 8.9.x version merge, should be alright

Apr 17 '24 05:04 rdlrt

Given the work done supporting docker builds on https://github.com/cardano-community/guild-operators/pull/1754 and the plans to skip the 8.9.x per https://github.com/cardano-community/guild-operators/pull/1747#issuecomment-2060371121 I believe the changes in this PR and it's target branch are no longer relevant to any release target.

I will test alpha on my fork and close this PR if all looks good and no one objects.

Apr 18 '24 00:04 Fuma419

Given the work done supporting docker builds on #1754 and the plans to skip the 8.9.x per #1747 (comment) I believe the changes in this PR and it's target branch are no longer relevant to any release target.

I will test alpha on my fork and close this PR if all looks good and no one objects.

I've added a commit for each fork-specific change you made into #1761.

EDIT: However, I missed your inclusion of nano as a text editor. If you prefer this to vi please feel free to open a PR to include it.

Apr 18 '24 00:04 TrevorBenson

Given the work done supporting docker builds on #1754 and the plans to skip the 8.9.x per #1747 (comment) I believe the changes in this PR and it's target branch are no longer relevant to any release target. I will test alpha on my fork and close this PR if all looks good and no one objects.

I've added a commit for each fork-specific change you made into #1761.

EDIT: However, I missed your inclusion of nano as a text editor. If you prefer this to vi please feel free to open a PR to include it.

I dont have strong opinions regarding nano vs vi, overall reducing the content/complexity of the image may be best.

Apr 18 '24 01:04 Fuma419

I believe the changes in this PR and it's target branch are no longer relevant to any release target.

Just to be sure it's not missed, I just meant 8.9.x PR will get re-created as 8.10.x branch / PR once a stable version is available. This is to avoid messy upgrades that ideally should have gone through a hard fork event (+ availability of stable component that is compatible across the board). So none of the work done in this or target branch will be lost

Apr 18 '24 06:04 rdlrt

Hmm - looks like 8.9.2 compatibility based update might be back on the table as new releases for dbsync (13.2.0.2), ogmios (6.2.0), node/cli are compatible. Will have to test, but if it works - will help reduce the delta of changes (and number of branches) on the repo

Apr 18 '24 12:04 rdlrt

Hey @Fuma419 @TrevorBenson , when you get a few - would you be able to resolve conflicts on this PR (apologies for the back-and-forth we've had on support for releases, we [maybe myself] were too optimistic about having a hardfork to gracefully handle the state on preprod network, but since that's not coming and we finally have release/tags for other components that do support 8.9.x, we have gone ahead with merge to alpha for node-8.9.0 branch)

Apr 21 '24 08:04 rdlrt

Hey @Fuma419 @TrevorBenson , when you get a few - would you be able to resolve conflicts on this PR (apologies for the back-and-forth we've had on support for releases, we [maybe myself] were too optimistic about having a hardfork to gracefully handle the state on preprod network, but since that's not coming and we finally have release/tags for other components that do support 8.9.x, we have gone ahead with merge to alpha for node-8.9.0 branch)

Sure, I resolved all but one file. @TrevorBenson could you look at the db-sync related conflict in scripts/cnode-helper-scripts/guild-deploy.sh? I think we want to take alpha but i'm not 100%.

ref: https://github.com/cardano-community/guild-operators/pull/1747/commits/339c12718ea2d36ce427e206f013039709a8b958

Apr 21 '24 14:04 Fuma419

this pipeline is failing on this command:

docker build .
--file files/tests/pre-merge/rockylinux-guild-deploy.sh-l.containerfile
--compress
--build-arg BRANCH=testing/node-8.9.1
--build-arg COMMIT=d20775e
--build-arg G_ACCOUNT=cardano-community
--tag ghcr.io/cardano-community/pre-merge-rockylinux:guild-deploy-l_d20775e

I see an issues in the --build-args because testing/node-8.9.1 does not exist in the cardano-community repo (it is a fork). Unsure what the best resolution. Can/should G_ACCOUNT parse the repo owner from repo url on each run?

Apr 21 '24 15:04 Fuma419

this pipeline is failing on this command:

docker build . --file files/tests/pre-merge/rockylinux-guild-deploy.sh-l.containerfile --compress --build-arg BRANCH=testing/node-8.9.1 --build-arg COMMIT=d20775e --build-arg G_ACCOUNT=cardano-community --tag ghcr.io/cardano-community/pre-merge-rockylinux:guild-deploy-l_d20775e

I see an issues in the --build-args because testing/node-8.9.1 does not exist in the cardano-community repo (it is a fork). Unsure what the best resolution. Can/should G_ACCOUNT parse the repo owner from repo url on each run?

I ran this locally with --build-arg G_ACCOUNT=fuma419 \ and it built successfully

Apr 21 '24 15:04 Fuma419

I implemented some logic (9f8894b and 82dfc8a) in the workflow that is failing. I was able to do some limited testing locally by:

Cloning the upstream, then adding the fork as a remote and checking out testing/node-8.9.1
Cloning the forked repo
running workflows in each repo with act (https://github.com/nektos/act)

While it did detect and set G_ACCOUNT in both of these tested scenarios, locally this does not simulate a fork merge properly, and i could not find a way to test a fork merge properly. Can this workflow be trigger to run again?

Apr 21 '24 17:04 Fuma419

I implemented some logic (9f8894b and 82dfc8a) in the workflow that is failing. I was able to do some limited testing locally by:

Cloning the upstream, then adding the fork as a remote and checking out testing/node-8.9.1

Cloning the forked repo

running workflows in each repo with act (https://github.com/nektos/act)

While it did detect and set G_ACCOUNT in both of these tested scenarios, locally this does not simulate a fork merge properly, and i could not find a way to test a fork merge properly. Can this workflow be trigger to run again?

There are a few different ways to trigger a workflow without using act, but each has drawbacks for what I think you probably are looking for. Forgive the verbosity below, but I figure giving you extra details might save you some time and frustration if you are unaware of the limits.

On the repository that ran the workflow go into Actions, click the failed job, and re-run the failed job.
- Changes to try and fix the workflow issue are not included. The first run checks out HEAD, but retry checks out the commit which was HEAD in the first failure.
- This is often confusing until comparing the checkout of the original workflow and the retry. The focus is to retry a failure of the CI, not a potential fix added to the branch after the failure.
- This is my experience from a few years ago, I doubt it has changed but I haven't reviewed it recently.
The workflow can be triggered manually via the Actions menu using the Run Workflow button, as the workflow has a workflow_dispatch option.
- However, the Branch to test workflow input is local to the repository where the Actions are being dispatched. So Fuma419:testing/node-8.9.1 isn't a viable branch to test since it only exists in the forked repository.
A new commit to the source branch (of the Forked repository) should kick off the workflow again in the upstream repository (base branch of the PR) due to the pull_request.
- I forget if this new commit has to be on one of the specific files in the path, or if any new commit in the branch will still cause it to run as the PR includes a change in prior commits to those files.
```
  pull_request:
    paths:
      - scripts/cnode-helper-scripts/guild-deploy.sh
      - scripts/cnode-helper-scripts/cabal-build-all.sh
      - files/tests/pre-merge/debian-guild-deploy.sh-l.containerfile
```
The simplest is generally to run the workflow from dispatch under Actions of the forked repository itself to test the premerge manually.
- Ignoring your current branch is named testing/ this is what I tend to do:
  1. git checkout issue/XXXX-blahblah
  2. git switch -C testing/XXXX-blahblah (or use git branch or whatever method you prefer to make a new branch from the current)
  3. Perform tests via actions workflow dispatch in the fork, commits to resolve issues, retest, rinse and repeat until happy.
  4. Optional: Do a rebase and reorder/squash/fixup the various commits together until I am happy.
  5. git switch issue/XXXX-blahblah (change back to the PR branch)
  6. Either: reset or cherry-pick, depends on steps taken in testing/XXXX-blahblah branch
    - git reset --hard testing/XXXX-blahblah
    - git cherry-pick <COMMIT_SHA>
  7. git push -f origin (force only needed for rebase with reset, if cherry picks juts add new commits then regular push w/o force is fine)

The 4th option ignores your branch is already named testing, in those cases I make a testing2 or forktesting etc. branch

Apr 22 '24 17:04 TrevorBenson

Hey @Fuma419 @TrevorBenson , when you get a few - would you be able to resolve conflicts on this PR (apologies for the back-and-forth we've had on support for releases, we [maybe myself] were too optimistic about having a hardfork to gracefully handle the state on preprod network, but since that's not coming and we finally have release/tags for other components that do support 8.9.x, we have gone ahead with merge to alpha for node-8.9.0 branch)

Sure, I resolved all but one file. @TrevorBenson could you look at the db-sync related conflict in scripts/cnode-helper-scripts/guild-deploy.sh? I think we want to take alpha but i'm not 100%.

ref: 339c127

I'll defer to @rdlrt or @Scitz0 on the db-sync related code or failures related to guild-deploy.sh. if they think this is specific to pre-merge tests and not general issues I'll take a closer look.

Apr 22 '24 17:04 TrevorBenson

I implemented some logic (9f8894b and 82dfc8a) in the workflow that is failing. I was able to do some limited testing locally by:

Cloning the upstream, then adding the fork as a remote and checking out testing/node-8.9.1

Cloning the forked repo

running workflows in each repo with act (https://github.com/nektos/act)

While it did detect and set G_ACCOUNT in both of these tested scenarios, locally this does not simulate a fork merge properly, and i could not find a way to test a fork merge properly. Can this workflow be trigger to run again?

There are a few different ways to trigger a workflow without using act, but each has drawbacks for what I think you probably are looking for. Forgive the verbosity below, but I figure giving you extra details might save you some time and frustration if you are unaware of the limits.
On the repository that ran the workflow go into Actions, click the failed job, and re-run the failed job.

Changes to try and fix the workflow issue are not included. The first run checks out HEAD, but retry checks out the commit which was HEAD in the first failure.

This is often confusing until comparing the checkout of the original workflow and the retry. The focus is to retry a failure of the CI, not a potential fix added to the branch after the failure.

This is my experience from a few years ago, I doubt it has changed but I haven't reviewed it recently.

The workflow can be triggered manually via the Actions menu using the Run Workflow button, as the workflow has a workflow_dispatch option.

However, the Branch to test workflow input is local to the repository where the Actions are being dispatched. So Fuma419:testing/node-8.9.1 isn't a viable branch to test since it only exists in the forked repository.
A new commit to the source branch (of the Forked repository) should kick off the workflow again in the upstream repository (base branch of the PR) due to the pull_request.

I forget if this new commit has to be on one of the specific files in the path, or if any new commit in the branch will still cause it to run as the PR includes a change in prior commits to those files.
  pull_request:
    paths:
      - scripts/cnode-helper-scripts/guild-deploy.sh
      - scripts/cnode-helper-scripts/cabal-build-all.sh
      - files/tests/pre-merge/debian-guild-deploy.sh-l.containerfile
The simplest is generally to run the workflow from dispatch under Actions of the forked repository itself to test the premerge manually.

Ignoring your current branch is named testing/ this is what I tend to do:

git checkout issue/XXXX-blahblah

git switch -C testing/XXXX-blahblah (or use git branch or whatever method you prefer to make a new branch from the current)

Perform tests via actions workflow dispatch in the fork, commits to resolve issues, retest, rinse and repeat until happy.

Optional: Do a rebase and reorder/squash/fixup the various commits together until I am happy.

git switch issue/XXXX-blahblah (change back to the PR branch)

Either: reset or cherry-pick, depends on steps taken in testing/XXXX-blahblah branch

git reset --hard testing/XXXX-blahblah

git cherry-pick <COMMIT_SHA>

git push -f origin (force only needed for rebase with reset, if cherry picks juts add new commits then regular push w/o force is fine)
The 4th option ignores your branch is already named testing, in those cases I make a testing2 or forktesting etc. branch

i appreciate the verbosity. im saving this info.

this last workflow change seems to have handled the fork properly, it's now failing due to a capital letter in my GitHub user account. I will make a small edit to address this edge case.
@rdlrt made me a contributor in guild-operators, which will be helpful, but I may continue to work from a fork if we think there is value, i am not sure, any opinions?
i see the rocky linux build also needs 'jq' added to the image deps. guild_deploy.sh is probably having some issues due to this. i can create an issue/pr.
can/should we add an ubuntu build to the workflow?

wait i see there is an ubuntu build

Apr 22 '24 18:04 Fuma419

i appreciate the verbosity. im saving this info.

this last workflow change seems to have handled the fork properly, it's now failing due to a capital letter in my GitHub user account. I will make a small edit to address this edge case.

@rdlrt made me a contributor in guild-operators, which will be helpful, but I may continue to work from a fork if we think there is value, i am not sure, any opinions?

Using my fork is nice when I want to add multiple commits and test before the final PR. From my fork I:

add cardano-community as upstream.
Then I push to origin (my fork) for each fix of failed tests from the CI.
Optional: After I'm happy and things pass tests I combine all the fixes into the related commits
- FWIW Using git rebase --autosquash <oldest COMMIT_SHA> with commits that used fixup! <message from commit you want to merge it into> makes quick work out of the rebase to clean the branch!
Next (for those who are contributors/collaborators which you now are) I do git push upstream to create the branch at cardano-comunity.
Finally, when I open the final PR if I want pre-merge testing etc. I use the source from cardano-community instead of from my fork.

It's easy to screw up the last step and open it from the fork, so it does take a little getting used to.

i see the rocky linux build also needs 'jq' added to the image deps. guild_deploy.sh is probably having some issues due to this. i can create an issue/pr.

:+1:

can/should we add an ubuntu build to the workflow?

wait i see there is an ubuntu build

Ubuntu is the default. Very few of us (myself included) use Enterprise Linux distros, so I built the rocky one just to make sure if something breaks along the way we catch it early.

Apr 23 '24 00:04 TrevorBenson

guild-operators guild-operators copied to clipboard

moving pr 1743 to node 8.9.1

Description

Where should the reviewer start?

Motivation and context

Which issue it fixes?

How has this been tested?

guild-operators
guild-operators copied to clipboard