-
Notifications
You must be signed in to change notification settings - Fork 53
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
HMS-4667: Blueprints: Allow importing without subscription #2434
Conversation
2c3962d
to
2ce878c
Compare
2ce878c
to
e7ae887
Compare
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #2434 +/- ##
==========================================
+ Coverage 75.71% 83.62% +7.91%
==========================================
Files 33 153 +120
Lines 597 17248 +16651
Branches 144 1668 +1524
==========================================
+ Hits 452 14424 +13972
- Misses 139 2805 +2666
- Partials 6 19 +13
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @avitova! 👍
I have a quick thought: Would it make sense to open the wizard directly on the subscription step when no subscription is present? This could help guide users more intuitively.
I've also left a few minor nitpicks.
await waitFor(async () => | ||
expect( | ||
await screen.findByText('Image output', { selector: 'h1' }) | ||
).toBeInTheDocument() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: The findByText
is already a promise that waits, so no need to wrap it in a WaitFor
await expect(
screen.findByText('Image output', { selector: 'h1' })
).resolves.toBeInTheDocument();
// or
const element = await screen.findByText('Image output', { selector: 'h1' });
expect(element).toBeInTheDocument();
subscriptionId: azureUploadOptions?.subscription_id || '', | ||
resourceGroup: azureUploadOptions?.resource_group, | ||
} | ||
: initialState.azure, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to use default values when no uploadOption exists. 👍
env: subscribed | ||
? { | ||
serverUrl: | ||
request.customizations.subscription?.['server-url'] || | ||
initialState.env.serverUrl, | ||
baseUrl: | ||
request.customizations.subscription?.['base-url'] || | ||
initialState.env.baseUrl, | ||
} | ||
: { | ||
serverUrl: '', | ||
baseUrl: '', | ||
}, | ||
registration: subscribed | ||
? { | ||
registrationType: request.customizations?.subscription | ||
? request.customizations.subscription.rhc | ||
? 'register-now-rhc' | ||
: 'register-now-insights' | ||
: initialState.registration.registrationType, | ||
activationKey: | ||
request.customizations.subscription?.['activation-key'] || | ||
initialState.registration.activationKey, | ||
} | ||
: { | ||
registrationType: 'register-later', | ||
activationKey: '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to make this more structure and readable, we could first create a subscriptionState
object, using the subscribed
flag just once.
const subscriptionState = subscribed
? {
serverUrl: request.customizations.subscription?.['server-url'] || initialState.env.serverUrl,
baseUrl: request.customizations.subscription?.['base-url'] || initialState.env.baseUrl,
registrationType: request.customizations.subscription
? request.customizations.subscription.rhc
? 'register-now-rhc'
: 'register-now-insights'
: initialState.registration.registrationType,
activationKey: request.customizations.subscription?.['activation-key'] || initialState.registration.activationKey,
}
: {
serverUrl: '',
baseUrl: '',
registrationType: 'register-later' as const,
activationKey: '',
};
and return
return {
// ...metadata...
env: {
serverUrl: subscriptionState.serverUrl,
baseUrl: subscriptionState.baseUrl,
},
registration: {
registrationType: subscriptionState.registrationType,
activationKey: subscriptionState.activationKey,
},
...commonRequestToState(blueprintResponse),
After discussions about the customer value of this solution, I am closing this pull request. Any details can be found in the backend pull request that is related to this one: ib backend #1343 |
This requires a small change on our backend - on export endpoint, we would need to export "subscription: {}" in the customizations field, so that we can detect that a subscription was there, when we import the blueprint back. :)
Let's not merge before we merge the backend change.
All information is in the ticket. But TLDR; we omit subscription now, and because of that, we can not detect if the blueprint was subscribed or not.