Merge pull request #4647 from cloud-gov/fix-build-url

fix: use single build url upon creation (#4323)
cloud-gov · Nov 4, 2024 · 646c090 · 646c090
2 parents 86e5768 + a75d060
commit 646c090
Show file tree

Hide file tree

Showing 14 changed files with 93 additions and 228 deletions.
diff --git a/admin-client/src/pages/Build.svelte b/admin-client/src/pages/Build.svelte
@@ -77,7 +77,7 @@
           <LabeledItem label="completed at" value={formatDateTime(build.completedAt, true)} title={formatDateTime(build.completedAt)} />
           <LabeledItem label="source" value={build.source} />
           <div>
-            <ExternalLink href={build.viewLink}>Live</ExternalLink>
+            <ExternalLink href={build.url}>Live</ExternalLink>
             <ExternalLink
               icon="github"
               href="https://github.com/{build.site.owner}/{build.site.repository}">
@@ -105,4 +105,4 @@
     overflow: auto;
     box-shadow: inset 0 0 3px gray;
   }
-</style>
+</style>
diff --git a/api/models/build.js b/api/models/build.js
@@ -133,6 +133,12 @@ const beforeValidate = (build) => {
 const sanitizeCompleteJobErrorMessage = (message) =>
   message.replace(/\/\/(.*)@github/g, '//[token_redacted]@github');
 
+async function beforeCreate(build) {
+  const { Site, SiteBranchConfig, Domain } = this.sequelize.models;
+  const site = await Site.findByPk(build.site, { include: [SiteBranchConfig, Domain] });
+  build.url = buildUrl(build, site);
+}
+
 const jobErrorMessage = (message = 'An unknown error occurred') =>
   sanitizeCompleteJobErrorMessage(message);
 
@@ -153,10 +159,6 @@ const jobStateUpdate = (buildStatus, build, site, timestamp) => {
     atts.completedAt = timestamp;
   }
 
-  if (buildStatus.status === States.Success) {
-    atts.url = buildUrl(build, site);
-  }
-
   if (build.canStart(buildStatus.status)) {
     atts.startedAt = timestamp;
   }
@@ -318,6 +320,7 @@ module.exports = (sequelize, DataTypes) => {
       tableName: 'build',
       hooks: {
         beforeValidate,
+        beforeCreate,
       },
       paranoid: true,
     },

diff --git a/api/serializers/build.js b/api/serializers/build.js
@@ -7,7 +7,6 @@ const {
   BuildTask,
   BuildTaskType,
 } = require('../models');
-const { buildViewLink } = require('../utils/build');
 const siteSerializer = require('./site');
 const userSerializer = require('./user');
 
@@ -25,17 +24,12 @@ const toJSON = (build) => {
     object.startedAt = object.startedAt.toISOString();
   }
 
-  if (build.Site) {
-    object.viewLink = buildViewLink(build, build.Site);
-  }
-
   Object.keys(object).forEach((key) => {
     if (object[key] === null) {
       delete object[key];
     }
   });
   delete object.token;
-  delete object.url;
   delete object.logsS3Key;
   // only return first 80 chars in case it's long
   if (object.error) {

diff --git a/api/services/GithubBuildHelper.js b/api/services/GithubBuildHelper.js
@@ -1,7 +1,6 @@
 const url = require('url');
 const GitHub = require('./GitHub');
 const config = require('../../config');
-const { buildViewLink } = require('../utils/build');
 
 // Loops through supplied list of users, until it
 // finds a user with a valid access token
@@ -109,7 +108,7 @@ const reportBuildStatus = async (build) => {
     options.description = 'The build is running.';
   } else if (build.state === 'success') {
     options.state = 'success';
-    options.target_url = buildViewLink(build, site);
+    options.target_url = build.url;
     options.description = 'The build is complete!';
   } else if (build.state === 'error') {
     options.state = 'error';

diff --git a/api/services/SiteBuildQueue.js b/api/services/SiteBuildQueue.js
@@ -9,7 +9,7 @@ const {
 } = require('../models');
 const config = require('../../config');
 const CloudFoundryAPIClient = require('../utils/cfApiClient');
-const { buildViewLink, buildUrl } = require('../utils/build');
+const { sitePrefix, buildUrl } = require('../utils/build');
 const GithubBuildHelper = require('./GithubBuildHelper');
 const S3Helper = require('./S3Helper');
 
@@ -24,13 +24,8 @@ const siteConfig = (build, siteBranchConfigs = []) => {
   return configRecord?.config || {};
 };
 
-const baseURLForDomain = (rawDomain) => url.parse(rawDomain).path.replace(/(\/)+$/, '');
-
-const sitePrefixForBuild = (rawDomain) =>
-  baseURLForDomain(rawDomain).replace(/^(\/)+/, '');
-
 const baseURLForBuild = (build) => {
-  const link = buildViewLink(build, build.Site);
+  const link = buildUrl(build, build.Site);
   const urlObject = new URL(link);
   return urlObject.pathname.replace(/(\/)+$/, '');
 };
@@ -61,7 +56,7 @@ const generateDefaultCredentials = async (build) => {
     CONFIG: JSON.stringify(siteConfig(build, SiteBranchConfigs)),
     REPOSITORY: repository,
     OWNER: owner,
-    SITE_PREFIX: sitePrefixForBuild(buildUrl(build, build.Site)),
+    SITE_PREFIX: sitePrefix(build, build.Site),
     GITHUB_TOKEN: (build.User || {}).githubAccessToken, // temp hot-fix
     GENERATOR: engine,
     BUILD_ID: build.id,

diff --git a/api/utils/build.js b/api/utils/build.js
@@ -1,56 +1,56 @@
 const config = require('../../config');
 
-function buildPath(build, site) {
-  const sbc = site?.SiteBranchConfigs?.find((c) => c.branch === build.branch);
-
-  if (sbc) {
-    return sbc.s3Key;
-  }
-
+function previewPath(build, site) {
   return `/preview/${site.owner}/${site.repository}/${build.branch}`;
 }
 
-function buildUrl(build, site) {
+function proxyUrl(path, site) {
   const { proxyDomain } = config.app;
-  const path = buildPath(build, site);
   const url = new URL(path, `https://${site.awsBucketName}.${proxyDomain}`);
-
-  return url.href;
+  // use a trailing slash to prevent the browser from interpreting
+  // branch names with dots as file extensions
+  return url.href + '/';
 }
 
-function buildViewLink(build, site) {
+/* canonical url for a build:
+  - if we don't have a matching site branch config, use the preview url
+  - if we don't have a matching domain, use the site branch config s3key path
+  - use the custom domain
+*/
+function buildUrl(build, site) {
   const { SiteBranchConfigs, Domains } = site;
 
-  if (build.url) {
-    const regex = /(http|https):\/\/+/;
-
-    if (regex.test(build.url)) {
-      return `${build.url.replace(regex, 'https://')}/`;
-    }
-
-    return `https://${build.url}/`;
-  }
-
   const siteBranchConfig = SiteBranchConfigs.find((sbc) => sbc.branch === build.branch);
 
   if (!siteBranchConfig) {
-    return `${buildUrl(build, site)}/`;
+    return proxyUrl(previewPath(build, site), site);
   }
 
   const domain = Domains.find(
     (d) => d.siteBranchConfigId === siteBranchConfig.id && d.state === 'provisioned',
   );
 
   if (!domain) {
-    return `${buildUrl(build, site)}/`;
+    return proxyUrl(siteBranchConfig.s3Key, site);
   }
 
   const domainName = domain.names.split(',')[0];
 
   return `https://${domainName.replace(/\/+$/, '')}/`;
 }
 
-module.exports = {
-  buildViewLink,
-  buildUrl,
-};
+/* generate SITE_PREFIX for builds
+ uses similar but slightly different logic to buildUrl
+(ignores domains, no proxyUrl, no leading slashes)
+*/
+function sitePrefix(build, site) {
+  const { SiteBranchConfigs } = site;
+
+  const siteBranchConfig = SiteBranchConfigs.find((sbc) => sbc.branch === build.branch);
+
+  const path = siteBranchConfig ? siteBranchConfig.s3Key : previewPath(build, site);
+
+  return path.replace(/^(\/)+/, '');
+}
+
+module.exports = { buildUrl, sitePrefix };
diff --git a/api/workers/jobProcessors/buildTaskRunner.js b/api/workers/jobProcessors/buildTaskRunner.js
@@ -21,28 +21,12 @@ async function buildTaskRunner(job, { sleepNumber = 15000, totalAttempts = 240 }
 
     let cfResponse;
 
-    // TODO: rewrite; some tasks rely on scanning the final build url:
-    // for non-production sites, this is buildTaskId.Build.url
-    // for production sites, we overwrite that value with the production domain
-    const siteBranchConfig = site.SiteBranchConfigs.find(
-      (sbc) => sbc.branch === buildTask.Build.branch,
-    );
-
     if (taskTypeRunner === BuildTaskType.Runners.Cf_task) {
       try {
         logger.log(
           `Starting ${taskTypeRunner} for ${owner}/${repository} on branch ${branch}`,
         );
-
-        const rawTask = buildTask.get({
-          plain: true,
-        });
-
-        if (siteBranchConfig?.Domains?.length) {
-          const domain = siteBranchConfig.Domains[0]; // TODO: always the first domain
-          rawTask.Build.url = `https://${domain.names.split(',')[0]}`;
-        }
-
+        const rawTask = buildTask.get({ plain: true });
         cfResponse = await apiClient.startBuildTask(rawTask, { data }, { sleepInterval });
 
         if (cfResponse.state === 'FAILED') {
@@ -76,9 +60,7 @@ async function buildTaskRunner(job, { sleepNumber = 15000, totalAttempts = 240 }
             BuildTask.Statuses.Created,
           ].includes(failedTask.status)
         ) {
-          await failedTask.update({
-            status: BuildTask.Statuses.Error,
-          });
+          await failedTask.update({ status: BuildTask.Statuses.Error });
         }
       }
 
@@ -103,10 +85,7 @@ async function buildTaskRunner(job, { sleepNumber = 15000, totalAttempts = 240 }
   } catch (err) {
     logger.log(`An error occured: ${err?.message}`);
     const errorTask = await BuildTask.findByPk(buildTaskId);
-    await errorTask.update({
-      status: BuildTask.Statuses.Error,
-      message: err?.message,
-    });
+    await errorTask.update({ status: BuildTask.Statuses.Error, message: err?.message });
     throw err;
   }
 }

diff --git a/frontend/pages/sites/$siteId/builds/Build.jsx b/frontend/pages/sites/$siteId/builds/Build.jsx
@@ -8,7 +8,7 @@ import buildActions from '@actions/buildActions';
 
 import GithubBuildBranchLink from '@shared/GithubBuildBranchLink';
 import GithubBuildShaLink from '@shared/GithubBuildShaLink';
-import BranchViewLink from '@shared/BranchViewLink';
+import { IconView } from '@shared/icons';
 import {
   IconCheckCircle,
   IconClock,
@@ -207,12 +207,15 @@ const Build = ({ build, latestForBranch, showBuildTasks = false, site }) => {
       <td data-title="Results" className="table-actions">
         {latestForBranch && build.state === 'success' && (
           <p className="site-link">
-            <BranchViewLink
-              branchName={build.branch}
-              site={site}
-              showIcon
-              completedAt={build.completedAt}
-            />
+            <a
+              href={build.url}
+              target="_blank"
+              rel="noopener noreferrer"
+              className={'view-site-link'}
+            >
+              <IconView />
+              View site preview
+            </a>
           </p>
         )}
         {latestForBranch && ['error', 'success'].includes(build.state) && (

diff --git a/frontend/pages/sites/$siteId/published/BranchRow.jsx b/frontend/pages/sites/$siteId/published/BranchRow.jsx
@@ -2,7 +2,7 @@ import React from 'react';
 import PropTypes from 'prop-types';
 import { Link } from 'react-router-dom';
 
-import BranchViewLink from '@shared/BranchViewLink';
+import BranchViewLink from './BranchViewLink';
 
 import { SITE } from '@propTypes';
 

diff --git a/frontend/shared/BranchViewLink.jsx → ...ites/$siteId/published/BranchViewLink.jsx b/frontend/shared/BranchViewLink.jsx → ...ites/$siteId/published/BranchViewLink.jsx
@@ -1,9 +1,7 @@
 import React from 'react';
 import PropTypes from 'prop-types';
-import { connect } from 'react-redux';
-import { SITE } from '../propTypes';
-import { IconView } from './icons';
-import globals from '../globals';
+import { SITE } from '@propTypes';
+import globals from '@globals';
 
 // Based on the site's related site branch configs and domains
 // This function determines the link for a built branch
@@ -32,17 +30,11 @@ const getViewLink = (branch, site) => {
   return `https://${domain.names.split(',')[0]}`;
 };
 
-export const BranchViewLink = ({ branchName, site, showIcon }) => {
+export const BranchViewLink = ({ branchName, site }) => {
   const domain = getViewLink(branchName, site);
 
   return (
-    <a
-      href={domain}
-      target="_blank"
-      rel="noopener noreferrer"
-      className={showIcon ? 'view-site-link' : ''}
-    >
-      {showIcon && <IconView />}
+    <a href={domain} target="_blank" rel="noopener noreferrer">
       View site preview
     </a>
   );
@@ -51,11 +43,5 @@ export const BranchViewLink = ({ branchName, site, showIcon }) => {
 BranchViewLink.propTypes = {
   branchName: PropTypes.string.isRequired,
   site: SITE.isRequired,
-  showIcon: PropTypes.bool,
 };
-
-BranchViewLink.defaultProps = {
-  showIcon: false,
-};
-
-export default connect()(BranchViewLink);
+export default BranchViewLink;
diff --git a/test/api/unit/models/build.test.js b/test/api/unit/models/build.test.js
@@ -296,7 +296,7 @@ describe('Build model', () => {
           expect(build.completedAt.getTime()).to.eql(site.publishedAt.getTime());
           const url = [
             `https://${site.awsBucketName}.${config.app.proxyDomain}`,
-            `/preview/${site.owner}/${site.repository}/${build.branch}`,
+            `/preview/${site.owner}/${site.repository}/${build.branch}/`,
           ].join('');
           expect(build.url).to.eql(url);
         });

diff --git a/test/api/unit/services/GithubBuildHelper.test.js b/test/api/unit/services/GithubBuildHelper.test.js
@@ -5,7 +5,7 @@ const config = require('../../../../config');
 const { Domain, Site, SiteBranchConfig, User } = require('../../../../api/models');
 const factory = require('../../support/factory');
 const githubAPINocks = require('../../support/githubAPINocks');
-const { buildViewLink } = require('../../../../api/utils/build');
+const { buildUrl } = require('../../../../api/utils/build');
 const GitHub = require('../../../../api/services/GitHub');
 const GithubBuildHelper = require('../../../../api/services/GithubBuildHelper');
 
@@ -770,6 +770,14 @@ describe('GithubBuildHelper', () => {
         await build.update({
           branch: 'preview-branch',
         });
+
+        await build.reload();
+
+        // this depends on branch so we update twice
+        await build.update({
+          url: buildUrl(build, site),
+        });
+
         const repoNock = githubAPINocks.repo({
           accessToken: user.githubAccessToken,
           owner: site.owner,
@@ -780,7 +788,7 @@ describe('GithubBuildHelper', () => {
           owner: site.owner,
           repo: site.repository,
           sha: clonedCommitSha,
-          targetURL: buildViewLink(build, site),
+          targetURL: buildUrl(build, site),
         });
 
         await build.reload({
@@ -791,6 +799,7 @@ describe('GithubBuildHelper', () => {
             },
           ],
         });
+
         await GithubBuildHelper.reportBuildStatus(build);
         expect(repoNock.isDone()).to.be.true;
         expect(statusNock.isDone()).to.be.true;