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

Correctly set site engine when adding a site #3761

Merged
merged 1 commit into from
Jan 27, 2022

Conversation

svenaas
Copy link
Contributor

@svenaas svenaas commented Jan 27, 2022

Changes proposed in this pull request:

  • Actually passes the engine parameter through when creating new sites.

This is intended to address #3753

security considerations

None

@svenaas
Copy link
Contributor Author

svenaas commented Jan 27, 2022

Weird. Failed on Concourse but not on local. It's one of these:

  1) Domain Service
       .updateSiteForProvisionedDomain()
         sets the domain name on the associated site if not previously set:
     Error: Timeout of 2500ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/app/test/api/unit/services/Domain.test.js)
      at listOnTimeout (node:internal/timers:557:17)
      at processTimers (node:internal/timers:500:7)

In the hopes that it is actually a timeout because Concourse is being slow, I am trying it again.

@svenaas
Copy link
Contributor Author

svenaas commented Jan 27, 2022

Well, shoot. Same timeout.

The tests still pass on my local machine (where, like Concourse, PRODUCT is set to 'Pages') as well as on CircleCI (where PRODUCT is set to 'Federalist').

The timeout happens in the DomainService unit test during a call to DomainService.updateSiteForProvisionedDomain which should have zero connection to the change this branch introduces in the site controller.

Any clever ideas, @18F/federalist-admins?

Copy link
Contributor

@davemcorwin davemcorwin left a comment

Choose a reason for hiding this comment

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

looks like something I probably borked....

@svenaas svenaas merged commit 4491f20 into staging Jan 27, 2022
@svenaas svenaas deleted the 3753-site-engine-do-not-erase branch January 27, 2022 21:56
@svenaas svenaas mentioned this pull request Feb 1, 2022
davemcorwin pushed a commit that referenced this pull request Jun 1, 2022
Correctly set site engine when adding a site
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.

None yet

2 participants