-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[TT-1847] keystone smoke use chainlink deployment #16123
[TT-1847] keystone smoke use chainlink deployment #16123
Conversation
…one-por-test-broken
…n that sets GH_TOKEN as env var available to tests
AER Report: CI Core ran successfully ✅AER Report: Operator UI CI ran successfully ✅ |
deployment/environment/devenv/don.go
Outdated
// set admin address for non-bootstrap nodes | ||
node.adminAddr = info.AdminAddr | ||
|
||
// TODO remove me, hack so that code can progress |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better comment: ~ "capability registry requires non-null admin address; use arbitrary default value if node is not configured"
deployment/environment/devenv/don.go
Outdated
JDId string // job distributor id returned by node after Job distributor is created in node | ||
Name string // name of the node | ||
AccountAddr map[uint64]string // chain id to node's account address mapping for supported chains | ||
PeerID string // peer id of the node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iirc there is already code to extract the peer id from the labels. what's the reason to promote it up? i'm concerned that it may not be available in all cases b/c the jd itself doesn't require it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed. But what about ocr key bundle id? I still need it, at least until we make JD work. Any chances to either keep it here or add to labels as well?
deployment/environment/devenv/don.go
Outdated
@@ -226,6 +237,7 @@ func (n *Node) CreateCCIPOCRSupportedChains(ctx context.Context, chains []JDChai | |||
if peerID == nil { | |||
return fmt.Errorf("no peer id found for node %s", n.Name) | |||
} | |||
n.PeerID = *peerID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iiuc, we are already erroring if no p2p, so always ok to promote the field to the top level? are there other use cases of the Node struct that don't set the PeerID?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea, didn't pay attention to labels. Removed my change.
@@ -116,6 +116,9 @@ type WorkflowConfig struct { | |||
ChainlinkCLI *ChainlinkCLIConfig `toml:"chainlink_cli"` | |||
UseExising bool `toml:"use_existing"` | |||
Existing *ExistingWorkflowConfig `toml:"existing"` | |||
DonID uint32 `toml:"don_id" validate:"required"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add comment about the intended value. is it supposed to be the same thing that comes from the cap registry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, after these heavy code modifications it remains unclear to me, how we link DONs to capabilities. Because that id is used to instruct the bootstrap node how to filter out workflows from the workflow registry. So okay, it now knows which workflow it needs to execute. It also knows the members of its workflow DON.
Ah, I think I see it. So when we call capabilities registry, we use hashed capability id and node's p2p id and that's the connection.
require.NotEmpty(t, os.Getenv(e2eJobDistributorImageEnvVarName), "missing env var: "+e2eJobDistributorImageEnvVarName) | ||
require.NotEmpty(t, os.Getenv(e2eJobDistributorVersionEnvVarName), "missing env var: "+e2eJobDistributorVersionEnvVarName) | ||
|
||
// disabled until we can figure out how to generate a gist read:write token in CI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expand this comment pls. in practice what does it mean for smth to be 'existing'? where does it need to exist and can you point to readme (in the comment) about how to create one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
maybe the naming isn't the most obvious, I am happy to change it, so that it's clearer.
NodeInfo: nodeInfo, | ||
} | ||
|
||
require.Len(t, bs.Nodes, 1, "expected only one node in the blockchain output") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true, changed it to >= 1, I originally added it just in case to avoid accessing an index that might not exist
capRegAddr, tx, capabilitiesRegistryInstance, err := cr_wrapper.DeployCapabilitiesRegistry(sc.NewTXOpts(), sc.Client) | ||
_, decodeErr := sc.Decode(tx, err) | ||
require.NoError(t, decodeErr, "failed to deploy capabilities registry contract") | ||
type keystoneContracts struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is a contract set abstraction. can we use it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
nodesToAdd := make([]cr_wrapper.CapabilitiesRegistryNodeParams, len(nodesInfo)-1) | ||
donNodes := make([][32]byte, len(nodesInfo)-1) | ||
func configureWorkflowDON(t *testing.T, ctfEnv *deployment.Environment, don *devenv.DON, chainSelector uint64) { | ||
kcrAllCaps := []keystone_changeset.DONCapabilityWithConfig{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pass cfg as input so other tests can reuse?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok to way for followup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you expand on that? Which config do you mean? Not this one I guess cfg keystone_changeset.InitialContractsCfg
?
Because the whole point of having this method is to wrap the creation of this config struct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i mean your are hardcoding the DONCapabilityWithConfig... iiuc and other tests will probably want to set this in a different way
…moke-use-chainlink-deployment
nodesToAdd, | ||
)) | ||
require.NoError(t, decodeErr, "failed to add nodes to capabilities registry") | ||
oracleConfig := keystone_changeset.OracleConfig{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any specific rationale for these values? if so pls comment, if not, pls comment where the inspiration came from
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
|
Requires
Supports