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

Jump to bottom

REALMC-10870: deploy draft after making deps and hosting changes #224

Closed
wants to merge 5 commits into from
Closed

REALMC-10870: deploy draft after making deps and hosting changes #224

wants to merge 5 commits into from

Conversation

jandersongo
Copy link
Contributor

@jandersongo jandersongo commented Nov 17, 2021 •
edited
Loading

Closing this ticket on account of successful manual testing

jandersongo
jandersongo commented Nov 17, 2021
View reviewed changes
internal/commands/push/command_test.go
Pushing changes
Deploying draft
Deployment complete`,
Pushing changes`,
"An error occurred while uploading hosting assets: failed to add /404.html: something bad happened",
"An error occurred while uploading hosting assets: failed to add /index.html: something bad happened",
} {
Copy link
Contributor Author

@jandersongo jandersongo Nov 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Now that deployment waits for dependencies and hosting assets, the "Deploying draft" and "Deployment complete" message will not appear if an error occurs for either (dependencies or hosting assets).
Would a separate "Deployment failed" message be appropriate here instead? or is the error message enough by itself?

arahmanan
arahmanan reviewed Nov 17, 2021
View reviewed changes
Copy link
Contributor

@arahmanan arahmanan left a comment

Choose a reason for hiding this comment

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

I added Nick to this review as well to get his thoughts on this as well. Did you manage to figure out why this used to work properly some times?

internal/commands/push/command.go Outdated
@@ -372,6 +367,16 @@ func (cmd *Command) Handler(profile *user.Profile, ui terminal.UI, clients cli.C
}
}

if len(appDiffs) > 0 || dependenciesDiffs.Len() > 0 && hostingDiffs.Size() > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if len(appDiffs) > 0 || dependenciesDiffs.Len() > 0 && hostingDiffs.Size() > 0 {
if len(appDiffs) > 0 || dependenciesDiffs.Len() > 0 || hostingDiffs.Size() > 0 {

internal/commands/push/command.go Outdated
@@ -266,31 +266,26 @@ func (cmd *Command) Handler(profile *user.Profile, ui terminal.UI, clients cli.C
return nil
}

if len(appDiffs) > 0 {
var draft realm.AppDraft
if len(appDiffs) > 0 || dependenciesDiffs.Len() > 0 && hostingDiffs.Size() > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this was my bad, but we should change this to:

Suggested change
if len(appDiffs) > 0 || dependenciesDiffs.Len() > 0 && hostingDiffs.Size() > 0 {
if len(appDiffs) > 0 || dependenciesDiffs.Len() > 0 || hostingDiffs.Size() > 0 {

internal/commands/push/command_test.go
@@ -807,9 +799,9 @@ Successfully pushed app up: eggcorn-abcde
assert.Equal(t, `Determining changes
Creating draft
Pushing changes
Installed dependencies
Deploying draft
Deployment complete
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we already have a test that confirms that the dependencies were properly deployed? In this test we just mock. the ImportDependenciesFn function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this test verifies that the dependencies are deployed:

realm-cli/internal/cloud/realm/dependencies_test.go

Line 29 in 9718baf

t.Run("should successfully import a zip node_modules archive", func(t *testing.T) {

@jandersongo
Copy link
Contributor Author

I added Nick to this review as well to get his thoughts on this as well. Did you manage to figure out why this used to work properly some times?

I was looking through the splunk logs to see why the command worked on dev and not on prod. I noticed that the order of calls to endpoints differed between the two as such:

DEV: (dev splunk)

  1. POST /import
  2. POST /dependencies/diff
  3. POST /import
  4. Alert SUCCESSFUL_DEPLOY
  5. POST /import
  6. POST /drafts
  7. POST /drafts/.../deployment/
  8. POST /import
  9. Alert SUCCESSFUL_DEPLOY
  10. PUT /dependencies
  11. GET /dependencies/status

PROD: (prod splunk)

  1. POST /import
  2. Alert SUCCESSFUL_DEPLOY
  3. POST /dependencies/diff
  4. POST /drafts
  5. POST /import
  6. POST /drafts/.../deployment
  7. Alert SUCCESSFUL_DEPLOY
  8. PUT /dependencies
  9. Alert SUCCESSFUL_DEPLOY
  10. GET /dependencies/status

I'm not sure why they differ but it looks like dev deploys after dealing with dependencies/diff whereas prod deploys before.

arahmanan
arahmanan approved these changes Nov 19, 2021
View reviewed changes
Copy link
Contributor

@arahmanan arahmanan left a comment

Choose a reason for hiding this comment

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

LGTM! Just a minor thing from my end.

internal/commands/push/command.go Outdated
@@ -266,31 +266,26 @@ func (cmd *Command) Handler(profile *user.Profile, ui terminal.UI, clients cli.C
return nil
}

if len(appDiffs) > 0 {
var draft realm.AppDraft
if len(appDiffs) > 0 || dependenciesDiffs.Len() > 0 || hostingDiffs.Size() > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I just realized that at this point we already know that one of these is true since we'll return early on line 231 if this wasn't true. So there is no need for this if statement.

internal/commands/push/command.go Outdated
@@ -372,6 +367,16 @@ func (cmd *Command) Handler(profile *user.Profile, ui terminal.UI, clients cli.C
}
}

if len(appDiffs) > 0 || dependenciesDiffs.Len() > 0 || hostingDiffs.Size() > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same goes for this if statement.

@arahmanan
Copy link
Contributor

@jandersongo any updates on this? We'll want this issue fixed before releasing this project.

@jandersongo
Copy link
Contributor Author

jandersongo commented Dec 1, 2021 •
edited
Loading

@jandersongo any updates on this? We'll want this issue fixed before releasing this project.

Actually, something unusual is happening. Before thanksgiving, I tried to reproduce the error with @makesitgo but it was somehow resolved (the dependencies were properly showing up after pushing a new app)? I also contacted the user who filed the HELP ticker and they said,
"the issue was really an edge case and the customer has already closed the case as they hit this issue by mistake." (here)
Yet, this week the error came back (the dependencies were missing after pushing a new app on prod). Nick & I are planning to do some further investigation either today or tomorrow to find out why there was this discrepancy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arahmanan arahmanan arahmanan approved these changes

@makesitgo makesitgo Awaiting requested review from makesitgo

@Amaan95 Amaan95 Awaiting requested review from Amaan95

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jandersongo @arahmanan