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

Add genereric command/params for bridge-relay #326

Merged
merged 2 commits into from
Jan 23, 2024

Conversation

PierreBesson
Copy link
Contributor

@PierreBesson PierreBesson commented Jan 23, 2024

Breaking change, revamp how the bridge relayer configuration is set.

@PierreBesson PierreBesson force-pushed the bridge-relay-generic-command branch 3 times, most recently from e928eab to 1292636 Compare January 23, 2024 10:35
@@ -33,6 +33,20 @@ spec:
image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}"
imagePullPolicy: {{ .Values.image.pullPolicy }}
args:
{{- if .Values.command }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Kubernetes has separate field for command, if we set command in args it will confuse users.

spec:
  containers:
  - name: command-demo-container
    command: ["printenv"]
    args: ["HOSTNAME", "KUBERNETES_PORT"]

currently the default value for command is ENTRYPOINT ["/home/user/bridge-entrypoint.sh"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how to call it. But we have multiple commands within the relayer binary. Maybe we can call it subcommand ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe let's use a list instead of strings/maps.

I see at least 2 use cases not covered by this PR:

  1. What will we do if we have 2 or 3 subcommands?
  2. How do we pass the list of valid bash parameters:
--lane 00000001 \
--lane 00000002 \
--dry-run

I assume it would be like this:

.Values.params:
  lane: '00000001'
  lane: '00000002'
  dry-run: ''

but this would result in invalid YAML (key lane present twice) and render output:

--lane=00000002 \
--dry-run=

I propose having a single .Values.args: [ ].
For example, for code like this:

subcommand1  \
subcommand2 \
subcommand3 \
--lane 00000001 \
--lane 00000002 \
--dry-run

we would have values:

.Values.args:
 - subcommand1
 - subcommand2
 - subcommand3
 - "--lane 00000001"
 - "--lane 00000002"
 - "--dry-run"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK let's go with your solution of only one "params" variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I implemented the single var solution. Also removed the previous way to set flags so this is a breaking change.

@BulatSaif
Copy link
Contributor

we already have Values.extraArgs to put any genereric command.

@PierreBesson
Copy link
Contributor Author

we already have Values.extraArgs to put any genereric command.

Yes but with these variables it makes it more clear what needs to be set...

@PierreBesson PierreBesson merged commit 307204d into main Jan 23, 2024
3 checks passed
@PierreBesson PierreBesson deleted the bridge-relay-generic-command branch January 23, 2024 14:06
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