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

Drop warning if region is not us-east-1 in mock #717

Merged
merged 1 commit into from
Aug 9, 2024

Conversation

stanhu
Copy link
Contributor

@stanhu stanhu commented Aug 7, 2024

Previously if Fog.mock! were used with some other region and the bucket already existed, a warning message, Your region <region> does not match the default region 'us-east-1'.

We were seeing a lot of these warnings in our test suite. As far as I can tell, there's no reason why some other region can't be used, so drop this.

Previously if `Fog.mock!` were used with some other region and
the bucket already existed, a warning message, `Your region <region>
does not match the default region 'us-east-1'".

We were seeing a lot of these warnings in our test suite. As far as I
can tell, there's no reason why some other region can't be used, so
drop this.
@stanhu stanhu force-pushed the sh-drop-mock-region-warning branch from 3e2ccf5 to e936356 Compare August 7, 2024 20:14
@stanhu
Copy link
Contributor Author

stanhu commented Aug 7, 2024

@geemus I think this logic has been in place for a long time, but I'm not sure why. Is this just to ensure that tests only use one well-defined region? I suppose tests could fail if buckets with the same name but different regions are used.

@geemus
Copy link
Member

geemus commented Aug 9, 2024

@stanhu good question. Presumably we had a reason at the time, but I certainly don't remember it now. I agree it seems pretty arbitrary and although it could lead to some issues with re-used bucket names it's not clear that is any worse than the current confusing and probably not very true-to-real mock behavior.

Copy link
Member

@geemus geemus left a comment

Choose a reason for hiding this comment

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

There was probably a reason for this, but it's lost to time. So let's go with the simpler/less confusing version.

@geemus geemus merged commit a8518f0 into fog:master Aug 9, 2024
8 checks passed
@geemus
Copy link
Member

geemus commented Aug 29, 2024

Released in v3.25.0.

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