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.

Sign up for GitHub

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

MERC-8592 Fix stencil CLI Push resulting in intermittent failures #935

Merged
merged 2 commits into from
Jun 6, 2022
Merged

MERC-8592 Fix stencil CLI Push resulting in intermittent failures #935

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
887a2f8
fix: raise an error when 409 happen while trying to upload a theme fo…
powerwlsl May 19, 2022
afdab92
fix: MERC-8592 Check if a theme is deleted before pushing a new theme
powerwlsl Jun 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/stencil-push.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ function stencilPush(options = {}, callback) {
utils.notifyUserOfThemeLimitReachedIfNecessary,
utils.promptUserToDeleteThemesIfNecessary,
utils.deleteThemesIfNecessary,
utils.checkIfDeletionIsComplete(),
utils.uploadBundleAgainIfNecessary,
utils.notifyUserOfThemeUploadCompletion,
utils.pollForJobCompletion((data) => ({ themeId: data.theme_id })),
Expand Down
46 changes: 45 additions & 1 deletion lib/stencil-push.utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ utils.uploadBundle = async (options) => {
config: { accessToken },
bundleZipPath,
storeHash,
uploadBundleAgain,
} = options;

const apiHost = options.apiHost || options.config.apiHost;
Expand All @@ -97,6 +98,7 @@ utils.uploadBundle = async (options) => {
apiHost,
bundleZipPath,
storeHash,
uploadBundleAgain,
});
return {
...options,
Expand Down Expand Up @@ -188,12 +190,54 @@ utils.deleteThemesIfNecessary = async (options) => {
return options;
};

utils.checkIfDeletionIsComplete = () => {
return async.retryable(
{
interval: 1000,
errorFilter: (err) => {
if (err.message === 'ThemeStillExists') {
console.log(`${'warning'.yellow} -- Theme still exists;Retrying ...`);
return true;
}
return false;
},
times: 5,
Copy link
Contributor

Choose a reason for hiding this comment

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

Talked offline. After we check the deletion status 5 times, the code is going to stop checking and fail with 409. We need to make sure to surface the proper error (deletion still in process) in this case.

Copy link
Contributor Author

@powerwlsl powerwlsl Jun 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

thanks for the code review! @bc-jcha

actually, it turns out the error raised is covered by a function called printCliResultError, so it doesn't go all the way and fail on 409.
image

So i think it's good to leave it as is. Or maybe we can change the error message more user friendly with better explanation.

},
utils.checkIfThemeIsDeleted(),
);
};

utils.checkIfThemeIsDeleted = () => async (options) => {
const {
themeLimitReached,
config: { accessToken },
storeHash,
themeIdsToDelete,
} = options;

if (!themeLimitReached) {
return options;
}

const apiHost = options.apiHost || options.config.apiHost;

const result = await themeApiClient.getThemes({ accessToken, apiHost, storeHash });

const themeStillExists = result.some((theme) => themeIdsToDelete.includes(theme.uuid));

if (themeStillExists) {
throw new Error('ThemeStillExists');
}

return options;
};

utils.uploadBundleAgainIfNecessary = async (options) => {
if (!options.themeLimitReached) {
return options;
}

return utils.uploadBundle(options);
return utils.uploadBundle({ ...options, uploadThemeAgain: true });
};

utils.notifyUserOfThemeUploadCompletion = async (options) => {
Expand Down
10 changes: 8 additions & 2 deletions lib/theme-api-client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,9 +278,10 @@ async function getVariationsByThemeId({ accessToken, apiHost, storeHash, themeId
* @param {string} options.accessToken
* @param {string} options.apiHost
* @param {string} options.storeHash
* @param {boolean} options.uploadBundleAgain
* @returns {Promise<object>}
*/
async function postTheme({ bundleZipPath, accessToken, apiHost, storeHash }) {
async function postTheme({ bundleZipPath, accessToken, apiHost, storeHash, uploadBundleAgain }) {
const formData = new FormData();
formData.append('file', fs.createReadStream(bundleZipPath), {
filename: path.basename(bundleZipPath),
Expand All @@ -303,7 +304,12 @@ async function postTheme({ bundleZipPath, accessToken, apiHost, storeHash }) {
// This status code is overloaded, so we need to check the message to determine
// if we want to trigger the theme deletion flow.
const uploadLimitPattern = /You have reached your upload limit/;
if (payload && payload.title && uploadLimitPattern.exec(payload.title)) {
if (
payload &&
payload.title &&
uploadLimitPattern.exec(payload.title) &&
!uploadBundleAgain
) {
return { themeLimitReached: true };
}

Expand Down