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

Move idls onto the main repo, rather than an unknown SHA, and sync with go module #6241

Merged
merged 9 commits into from
Aug 20, 2024

Conversation

Groxx
Copy link
Member

@Groxx Groxx commented Aug 20, 2024

idls/ and the github.com/uber/cadence-idl version in go.mod are currently out of sync and more than a little screwed up:

  • the go module is on 0482c040f91d17be35cccee2bc1cf883f7344992
  • the IDL submodule is on 1cd936eb8e24d42f8d8cd38d6216e619c14c48d5

The go module is currently on cadence-workflow/cadence-idl@0482c04
which is the latest master and seems fine.

The current IDL submodule: https://github.com/uber/cadence-idl/tree/1cd936eb8e24d42f8d8cd38d6216e619c14c48d5
doesn't belong to any branch (much less master), and is in danger of being GC'd if we don't do something about that.

It's probably from a temporary PR branch that was cleaned up as part of #6178 (the contents are similar to https://github.com/uber/cadence-idl/tree/d92bb53292d64b698c0c5600bbd6e258e728a020 which was approved and is on master), but I can't find 1cd936eb8e24d42f8d8cd38d6216e619c14c48d5 in any PR in either https://github.com/uber/cadence-idl or https://github.com/timl3136/cadence-idl so I'm really not sure.

So this PR contains a few changes:

  • moves idls/ to match go.mod SHA: 0482c040f91d17be35cccee2bc1cf883f7344992
    • d92bb53292d64b698c0c5600bbd6e258e728a020 did not build, as the service code for ListAll... was removed, which is also why I didn't just revert the related commits.
  • adds a make .idl-status and CI steps to verify that this all stays in sync in the future, because this is all VERY easy to miss in PR reviews, and github's UI just makes it worse by hiding many of these details.

Tested with:

works both locally and in CI on this PR:

$ make .idl-status
branches="$(git submodule foreach git branch master --contains HEAD)"; \
if ! (echo "$branches" | grep -q master); then \
  >&2 echo "IDL submodule points to a commit ($(git submodule foreach git rev-parse HEAD | tail -n 1)) that is not on master."; \
  >&2 echo "Make sure the IDL PR has been merged, and this PR is updated, before merging here."; \
  exit 1; \
fi
idlsha="$(git ls-tree HEAD idls | awk '{print substr($3,0,12)}')"; \
gosha="$(grep github.com/uber/cadence-idl go.mod | tr '-' '\n' | tail -n1)"; \
if [[ "$idlsha" != "$gosha" ]]; then \
  >&2 echo "IDL submodule sha ($idlsha) does not match go module sha ($gosha)."; \
  >&2 echo "Make sure the IDL PR has been merged, and this PR is updated, before merging here."; \
  exit 1; \
fi
# Successfully shut down Job API server

and failing on master for the submodule issue:

❯ make .idl-status
branches="$(git submodule foreach git branch master --contains HEAD)"; \
	if ! (echo "$branches" | grep -q master); then \
	  >&2 echo "IDL submodule points to a commit ($(git submodule foreach git rev-parse HEAD | tail -n 1)) that is not on master."; \
	  >&2 echo "Make sure the IDL PR has been merged, and this PR is updated, before merging here."; \
	  exit 1; \
	fi
IDL submodule points to a commit (1cd936eb8e24d42f8d8cd38d6216e619c14c48d5) that is not on master.
Make sure the IDL PR has been merged, and this PR is updated, before merging here.
make: *** [.idl-status] Error 1

and the go.mod drift when the git submodule is updated:

❯ make .idl-status
branches="$(git submodule foreach git branch master --contains HEAD)"; \
	if ! (echo "$branches" | grep -q master); then \
	  >&2 echo "IDL submodule points to a commit ($(git submodule foreach git rev-parse HEAD | tail -n 1)) that is not on master."; \
	  >&2 echo "Make sure the IDL PR has been merged, and this PR is updated, before merging here."; \
	  exit 1; \
	fi
idlsha="$(git ls-tree HEAD idls | awk '{print substr($3,0,12)}')"; \
	gosha="$(go list -m github.com/uber/cadence-idl | tr '-' '\n' | tail -n1)"; \
	if [[ "$idlsha" != "$gosha" ]]; then \
	  >&2 echo "IDL submodule sha ($idlsha) does not match go module sha ($gosha)."; \
	  >&2 echo "Make sure the IDL PR has been merged, and this PR is updated, before merging here."; \
	  exit 1; \
	fi
IDL submodule sha (d92bb53292d6) does not match go module sha (0482c040f91d).
Make sure the IDL PR has been merged, and this PR is updated, before merging here.
make: *** [.idl-status] Error 1

CI only checks the committed state so it's consistent and obvious in its behavior, but locally it can get a bit confused about non-committed differences.
This could be fixed, but... tbh I don't think it matters much. PRs will always have committed values, and this really isn't local-run-intended because it's common for it to be out of sync while developing.

Copy link

codecov bot commented Aug 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.93%. Comparing base (67fcf12) to head (b8e3cb1).
Report is 1 commits behind head on master.

Additional details and impacted files

see 7 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67fcf12...b8e3cb1. Read the comment docs.

It's probably worth building a linter for this, though it shouldn't block other CI steps...

check for merged idl commit

no [[ apparently

debugging

maybe this avoids yaml issues

tmp

move logic to a makefile

restore the bad idl checkout

trying older sha

better failure message

better output

go back temporarily
@Groxx Groxx changed the title Move idls onto the main repo, rather than a fork Move idls onto the main repo, rather than an unknown SHA, and sync with go module Aug 20, 2024
# and last but not least: this avoids using `go` to make this check take only a couple seconds in CI,
# so the whole docker container doesn't have to be prepared.
.idl-status:
branches="$$(git submodule foreach git branch master --contains HEAD)"; \
Copy link
Member

@davidporter-id-au davidporter-id-au Aug 20, 2024

Choose a reason for hiding this comment

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

does this work? I get some spurious output:

git submodule foreach git branch master --contains HEAD | grep id
Entering 'idls'

I would assume this would mean that $branches was equal to Entering ids\n

Copy link
Member

Choose a reason for hiding this comment

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

oh, I didn't read the line below correctly, you're checking for the presence of the master branch (whcih I currently don't have). ok, np

Copy link
Member Author

@Groxx Groxx Aug 20, 2024

Choose a reason for hiding this comment

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

yea. "working" is

Entering 'idls'
 master

and "not" is

Entering 'idls'

git submodule foreach is just a bit easier than cd-ing around and dealing with those details, since I couldn't find a way to tell git to do "in this submodule only, do X". though the output is rather annoying at a glance.

if we ever have 2+ submodules I may have to change this, but that seems less likely than us getting rid of this one submodule eventually.

Copy link
Member

@davidporter-id-au davidporter-id-au left a comment

Choose a reason for hiding this comment

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

this is a good change to have

@Groxx Groxx merged commit 3e3b40c into cadence-workflow:master Aug 20, 2024
20 checks passed
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.

2 participants