Skip to content

feat(hybrid-cloud): Adds region selector dropdown behind feature flag#59164

Merged
GabeVillalobos merged 6 commits into
masterfrom
hybrid-cloud-dropdown-selector
Nov 6, 2023
Merged

feat(hybrid-cloud): Adds region selector dropdown behind feature flag#59164
GabeVillalobos merged 6 commits into
masterfrom
hybrid-cloud-dropdown-selector

Conversation

@GabeVillalobos

@GabeVillalobos GabeVillalobos commented Oct 31, 2023

Copy link
Copy Markdown
Member

Implements a simple dropdown with default region mapping for our two expected regions, guarded behind a feature flag.

Automatically routes the provision request to the correct region when one is selected via the dropdown.

Without the feature flag enabled, or with it enabled in monolith mode:

image

With the feature flag enabled and with multiple regions:

image
image

@github-actions github-actions Bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Oct 31, 2023
@GabeVillalobos GabeVillalobos requested review from a team October 31, 2023 23:55
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Oct 31, 2023
@github-actions

Copy link
Copy Markdown
Contributor

🚨 Warning: This pull request contains Frontend and Backend changes!

It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently.

Have questions? Please ask in the #discuss-dev-infra channel.

Comment thread static/app/views/organizationCreate/index.tsx
@codecov

codecov Bot commented Nov 1, 2023

Copy link
Copy Markdown

Codecov Report

Merging #59164 (8956d30) into master (3a76704) will increase coverage by 0.35%.
Report is 57 commits behind head on master.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master   #59164      +/-   ##
==========================================
+ Coverage   80.39%   80.75%   +0.35%     
==========================================
  Files        5165     5164       -1     
  Lines      225901   225928      +27     
  Branches    38021    38022       +1     
==========================================
+ Hits       181613   182448     +835     
+ Misses      38645    37948     -697     
+ Partials     5643     5532     -111     
Files Coverage Δ
src/sentry/options/defaults.py 100.00% <100.00%> (ø)
static/app/components/forms/apiForm.tsx 72.72% <ø> (-4.20%) ⬇️
static/app/views/organizationCreate/index.tsx 83.33% <ø> (ø)
src/sentry/web/client_config.py 84.61% <0.00%> (+3.78%) ⬆️

... and 131 files with indirect coverage changes

Comment thread static/app/views/organizationCreate/index.tsx Outdated
Comment thread static/app/views/organizationCreate/index.tsx
Comment thread static/app/views/organizationCreate/index.tsx Outdated
Comment thread static/app/components/forms/apiForm.tsx Outdated
addLoadingMessage(t('Saving changes\u2026'));
api.request(apiEndpoint, {

const request_options: RequestOptions = {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
const request_options: RequestOptions = {
const requestOptions: RequestOptions = {

Comment thread static/app/components/forms/apiForm.tsx Outdated
Comment on lines +42 to +45
request_options.host = hostOverride;
}

api.request(apiEndpoint, request_options);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
request_options.host = hostOverride;
}
api.request(apiEndpoint, request_options);
requestOptions.host = hostOverride;
}
api.request(apiEndpoint, requestOptions);

To align better with naming conventions elsewhere in our JS code.

ConfigStore.get('privacyUrl');

// Set only a single region in the config store by default
ConfigStore.set('regions', [{name: '--monolith--', url: 'https://example.com'}]);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You might be leaking state here. Should we backup state at the beginning of beforeEach() and restore in an afterEach?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point, I'll go ahead and add that. Do we have global setup/teardown fixtures for ConfigStore by chance?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we have global setup/teardown fixtures for ConfigStore by chance?

I don't think we have global setup/teardown on any of our reflux stores.

return [url, RegionDisplayName[regionName]];
}

return [url, name];

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we're sending the host name for the region in the request, do we parse the URL in the server side? Should we be sending the region name instead so that we don't have to parse URLs from user data?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

As far as I know, the region is unused/extraneous in the request body, but I couldn't figure out how to omit it from the form data 😅 Ideally we wouldn't send this at all.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We could supply a model to ApiForm that omits the region field from the serialized request data via the transformData method.

getTransformedData() {
const form = this.getData();
const data = Object.keys(form)
.map(id => [id, this.getTransformedValue(id)])
.reduce<Record<string, any>>((acc, [id, value]) => {
acc[id] = value;
return acc;
}, {});
return this.options.transformData ? this.options.transformData(data, this) : data;

is the signature for FormModel.transformData and an existing usage can be found here

function transformData(data: Record<string, any>, model: FormModel) {
// map object to list of questions
const questionnaire = Array.from(model.fieldDescriptor.values()).map(field =>
// we read the meta for the question that has a react node for the label
({
question: field.meta || field.label,
answer: data[field.name],
})
);
return {questionnaire};
}
type Props = ModalRenderProps & {
app: SentryApp;
};
export default class SentryAppPublishRequestModal extends Component<Props> {
form = new FormModel({transformData});

@GabeVillalobos GabeVillalobos merged commit 03b66f8 into master Nov 6, 2023
@GabeVillalobos GabeVillalobos deleted the hybrid-cloud-dropdown-selector branch November 6, 2023 17:23
@github-actions github-actions Bot locked and limited conversation to collaborators Nov 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Backend Automatically applied to PRs that change backend components Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants