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

Remove default host and tidy comments #382

Merged
merged 1 commit into from
Nov 29, 2024

Conversation

tofu-rocketry
Copy link
Member

Resolves #375 in a slightly different way to suggested.

We don't want people accidentally sending to the production host, but we also don't want people to accidentally send to the devel host if not intended. So remove the default host and add the list of hosts to a comment, explaining the purpose of each.

Also uncomment the port option and clarify its usage. It does nothing for AMS sending, so might as well leave it uncommented.

@tofu-rocketry tofu-rocketry added this to the 4.0.0 milestone Nov 28, 2024
@tofu-rocketry tofu-rocketry self-assigned this Nov 28, 2024
@tofu-rocketry tofu-rocketry requested a review from a team as a code owner November 28, 2024 16:56
Copy link
Member

@Sae126V Sae126V left a comment

Choose a reason for hiding this comment

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

Make sense, Adrian. LGTM.

@Sae126V
Copy link
Member

Sae126V commented Nov 29, 2024

-> 'port' is only used for STOMP sending.
port: 443

Just one thing, Is this intentional, Adrian. If so, We might need to make a note to update common/apel/metaconfig/schema.pan in AQ under sender_cfg_broker_struct -> port ? string to required.

@tofu-rocketry
Copy link
Member Author

tofu-rocketry commented Nov 29, 2024

-> 'port' is only used for STOMP sending. port: 443

Just one thing, Is this intentional, Adrian. If so, We might need to make a note to update common/apel/metaconfig/schema.pan in AQ under sender_cfg_broker_struct -> port ? string to required.

The port options are already optional in the AQ config for SSM, aren't they? So that's fine, right?

Oh, I misread. We only use AMS sending in production, so no need to change the AQ config.

We don't want people accidentally sending to the production host, but we
also don't want people to accidentally send to the devel host if not
intended. So remove the default host and add the list of hosts to a
comment, explaining the purpose of each.

Also uncomment the port option and clarify its usage. It does nothing
for AMS sending, so might as well leave it uncommented.
@tofu-rocketry tofu-rocketry merged commit e30d6f5 into apel:dev Nov 29, 2024
11 checks passed
@tofu-rocketry tofu-rocketry deleted the sender-config branch November 29, 2024 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Comment out AMS hosts in config
2 participants