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

Removed '$' sign from region check in publish_lambda.sh #727

Merged
merged 5 commits into from
Sep 26, 2024

Conversation

hvltaj
Copy link
Contributor

@hvltaj hvltaj commented Jun 5, 2024

Bug

What does this PR do?

Fixed bug in the publish_lambda.sh script which always set up gov as bucket region.

Why is it important?

Deployment script only works for gov regions in current state

Checklist

  • [~~] My code follows the style guidelines of this project
  • [~~] I have commented my code, particularly in hard-to-understand areas
  • [~~] I have made corresponding changes to the documentation
  • [~~] I have made corresponding change to the default configuration files
  • [~~] I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.md

Author's Checklist

How to test this PR locally

Use deployment script to deploy Lambda.

Related issues

Closes #726

Use cases

Screenshots

Logs

Copy link

cla-checker-service bot commented Jun 5, 2024

💚 CLA has been signed

@zmoog zmoog self-assigned this Sep 17, 2024
@zmoog zmoog added bug Something isn't working Team:obs-ds-hosted-services labels Sep 17, 2024
@zmoog
Copy link
Contributor

zmoog commented Sep 17, 2024

Hey @hvltaj, thanks for contributing the fix.

Adding this to the next release.

CHANGELOG.md Outdated
@@ -1,3 +1,7 @@
### v1.17.1 - 2024/09/17
Copy link
Contributor

Choose a reason for hiding this comment

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

This version should match the one inside share/version.py:

Perhaps we should update the workflow to compare these, this should not pass...

Copy link
Contributor

Choose a reason for hiding this comment

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

But updating the version.py will also trigger a new release, which seems a bit unnecessary, since the dependencies are not changing (the dependencies can be seen here).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, @constanca-m, you're right.

@hvltaj, the publish_lambda.sh is not part of the release; it's a supporting tool, so we can avoid updating the CHANGELOG.md and share/version.py files.

Copy link
Contributor

Choose a reason for hiding this comment

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

I updated the changelog after #803 is merged. Now it should match the version inside share/version.py.

Copy link
Contributor

Choose a reason for hiding this comment

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

ahh good point, ok I will take out the entry from CHANGELOG.md then!

Copy link
Contributor

Choose a reason for hiding this comment

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

I have created #807 to see how we can stop this from happening, or maybe ease this version update process a bit.

@kaiyan-sheng kaiyan-sheng requested a review from zmoog September 23, 2024 16:01
@zmoog zmoog merged commit c33221c into elastic:main Sep 26, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Team:obs-ds-hosted-services
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue with region name check in publishing script
4 participants