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

ARC-814 RepoSyncState table as source of truth #928

Merged
merged 10 commits into from
Dec 16, 2021
Merged

ARC-814 RepoSyncState table as source of truth #928

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
19015cc
initial commit for new repo table as source
mboudreau Nov 22, 2021
51e5c2f
adding more tests for reposyncstate
mboudreau Nov 24, 2021
99e7967
fixing API tests
mboudreau Nov 24, 2021
8d69168
fixing more tests that are using RepoSyncState table
mboudreau Nov 24, 2021
b26d52c
merging with main, updating snapshots
mboudreau Nov 24, 2021
747f037
merging main
mboudreau Dec 9, 2021
51922a3
merging main
mboudreau Dec 15, 2021
cdfca3d
Merge branch 'main' into ARC-814-reposyncstate-as-source
mboudreau Dec 15, 2021
cd0e9b9
Merge branch 'main' into ARC-814-reposyncstate-as-source
mboudreau Dec 16, 2021
0c76126
Merge branch 'main' into ARC-814-reposyncstate-as-source
mboudreau Dec 16, 2021
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
11 changes: 10 additions & 1 deletion package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@
"uuid": "^8.3.2"
},
"devDependencies": {
"@types/jest-when": "^2.7.3",
"@typescript-eslint/eslint-plugin": "~4.23.0",
"@typescript-eslint/parser": "~4.23.0",
"dotenv-cli": "^4.0.0",
Expand Down
32 changes: 16 additions & 16 deletions src/api/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import RedisStore from "rate-limit-redis";
import Redis from "ioredis";
import BodyParser from "body-parser";
import GithubAPI from "../config/github-api";
import { Installation, Subscription } from "../models";
import { Installation, RepoSyncState, Subscription } from "../models";
import verifyInstallation from "../jira/verify-installation";
import logMiddleware from "../middleware/frontend-log-middleware";
import JiraClient from "../models/jira-client";
Expand All @@ -18,6 +18,7 @@ import { getLogger } from "../config/logger";
import { Job, Queue } from "bull";
import { WhereOptions } from "sequelize";
import getJiraClient from "../jira/client";
import { booleanFlag, BooleanFlags } from "../config/feature-flags";

const router = express.Router();
const bodyParser = BodyParser.urlencoded({ extended: false });
Expand Down Expand Up @@ -168,7 +169,11 @@ router.get(
return;
}

res.json(subscription.repoSyncState);
if(await booleanFlag(BooleanFlags.REPO_SYNC_STATE_AS_SOURCE, false)) {
mboudreau marked this conversation as resolved.
Show resolved Hide resolved
res.json(await RepoSyncState.toRepoJson(subscription));
} else {
res.json(subscription.repoSyncState);
}
} catch (err) {
res.status(500).json(err);
}
Expand Down Expand Up @@ -382,23 +387,18 @@ router.post(
bodyParser,
check("installationId").isInt(),
returnOnValidationError,
async (req: Request, response: Response): Promise<void> => {
async (req: Request, res: Response): Promise<void> => {
const { installationId } = req.params;
const installation = await Installation.findByPk(installationId);

const isValid = await verifyInstallation(installation, req.log)();
const respondWith = (message) =>
response.json({
message,
installation: {
enabled: isValid,
id: installation.id,
jiraHost: installation.jiraHost
}
});
respondWith(
isValid ? "Verification successful" : "Verification failed"
);
res.json({
message: isValid ? "Verification successful" : "Verification failed",
installation: {
enabled: isValid,
id: installation.id,
jiraHost: installation.jiraHost
}
})
}
);

Expand Down
1 change: 1 addition & 0 deletions src/config/feature-flags.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ export enum BooleanFlags {
SUPPORT_BRANCH_AND_MERGE_WORKFLOWS_FOR_BUILDS = "support-branch-and-merge-workflows-for-builds",
USE_NEW_GITHUB_CLIENT_FOR_PUSH = "use-new-github-client-for-push",
USE_NEW_GITHUB_CLIENT_TO_COUNT_REPOS = "use-new-github-client-to-count-repos",
REPO_SYNC_STATE_AS_SOURCE = "repo-sync-state-as-source",
CALL_IS_ADMIN_AS_APP = "call-is-admin-as-app"
}

Expand Down
64 changes: 36 additions & 28 deletions src/frontend/get-jira-configuration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
import format from "date-fns/format";
import moment from "moment";
import { Subscription } from "../models";
import SubscriptionClass from "../models/subscription";
import SubscriptionClass, { SyncStatus } from "../models/subscription";
import { RepoSyncState, Subscription } from "../models";
import { NextFunction, Request, Response } from "express";
import statsd from "../config/statsd";
import { metricError } from "../config/metric-names";
import { FailedInstallations } from "../config/interfaces";
import { booleanFlag, BooleanFlags } from "../config/feature-flags";
import Logger from "bunyan";

function mapSyncStatus(syncStatus: string): string {
function mapSyncStatus(syncStatus: SyncStatus = SyncStatus.PENDING): string {
Copy link
Contributor

Choose a reason for hiding this comment

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

In which case this method will have to use the default parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Subscription.syncStatus is optional, we needed a way to handle in the off chance that it wasn't set yet, but almost certainly would be by this point as this is for current subscriptions which any new subscriptions is automatically synced and set to PENDING. https://github.com/atlassian/github-for-jira/blob/main/src/models/subscription.ts#L166

switch (syncStatus) {
case "ACTIVE":
return "IN PROGRESS";
Expand All @@ -18,23 +20,26 @@ function mapSyncStatus(syncStatus: string): string {
}
}

export async function getInstallation(client, subscription, reqLog?) {
export async function getInstallation(client, subscription: SubscriptionClass, reqLog?: Logger) {
const id = subscription.gitHubInstallationId;
try {
const response = await client.apps.getInstallation({ installation_id: id });
response.data.syncStatus = mapSyncStatus(subscription.syncStatus);
response.data.syncWarning = subscription.syncWarning;
response.data.subscriptionUpdatedAt = formatDate(subscription.updatedAt);
response.data.totalNumberOfRepos = Object.keys(
subscription.repoSyncState?.repos || {}
).length;
response.data.numberOfSyncedRepos =
subscription.repoSyncState?.numberOfSyncedRepos || 0;
if (await booleanFlag(BooleanFlags.REPO_SYNC_STATE_AS_SOURCE, false, subscription.jiraHost)) {
response.data.totalNumberOfRepos = await RepoSyncState.countFromSubscription(subscription);
response.data.numberOfSyncedRepos = await RepoSyncState.countSyncedReposFromSubscription(subscription);
} else {
response.data.totalNumberOfRepos = Object.keys(subscription.repoSyncState?.repos || {}).length;
response.data.numberOfSyncedRepos = subscription.repoSyncState?.numberOfSyncedRepos || 0;
}

response.data.jiraHost = subscription.jiraHost;

return response.data;
} catch (err) {
reqLog.error(
reqLog?.error(
{ installationId: id, error: err, uninstalled: err.code === 404 },
"Failed connection"
);
Expand All @@ -44,10 +49,10 @@ export async function getInstallation(client, subscription, reqLog?) {
}
}

const formatDate = function (date) {
const formatDate = function(date) {
return {
relative: moment(date).fromNow(),
absolute: format(date, "MMMM D, YYYY h:mm a"),
absolute: format(date, "MMMM D, YYYY h:mm a")
};
};

Expand All @@ -57,27 +62,30 @@ interface FailedConnections {
orgName: string | undefined;
}

export const getFailedConnections = (
export const getFailedConnections = async (
installations: FailedInstallations[],
subscriptions: SubscriptionClass[]
): FailedConnections[] => {
return installations
): Promise<FailedConnections[]> => {
return Promise.all(installations
.filter((response) => !!response.error)
.map((failedConnection: FailedInstallations) => {
const sub = subscriptions.find(
(subscription: SubscriptionClass) =>
failedConnection.id === subscription.gitHubInstallationId
);
const repos = sub?.repoSyncState?.repos || {};
const repoId = Object.keys(repos);
const orgName = repos[repoId[0]]?.repository?.owner.login || undefined;
.map(async (failedConnection: FailedInstallations) => {
const sub = subscriptions.find(sub => failedConnection.id === sub.gitHubInstallationId);
let orgName;
if (sub && await booleanFlag(BooleanFlags.REPO_SYNC_STATE_AS_SOURCE, false, sub.jiraHost)) {
const repo = await RepoSyncState.findOneFromSubscription(sub);
orgName = repo.repoOwner;
} else {
const repos = sub?.repoSyncState?.repos || {};
mboudreau marked this conversation as resolved.
Show resolved Hide resolved
const repoId = Object.keys(repos);
orgName = repos[repoId[0]]?.repository?.owner.login || undefined;
}

return {
id: failedConnection.id,
deleted: failedConnection.deleted,
orgName,
orgName
};
});
}));
};

export default async (
Expand All @@ -104,7 +112,7 @@ export default async (
)
);

const failedConnections = getFailedConnections(
const failedConnections = await getFailedConnections(
Copy link
Contributor

Choose a reason for hiding this comment

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

does this mean failedConnections has always been an unresolved promise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it used to be a sync function which I changed to async for the booleanFlag call

installations,
subscriptions
);
Expand All @@ -116,7 +124,7 @@ export default async (
isGlobalInstall: data.repository_selection === "all",
installedAt: formatDate(data.updated_at),
syncState: data.syncState,
repoSyncState: data.repoSyncState,
repoSyncState: data.repoSyncState
}));

const hasConnections =
Expand All @@ -129,7 +137,7 @@ export default async (
hasConnections,
APP_URL: process.env.APP_URL,
csrfToken: req.csrfToken(),
nonce: res.locals.nonce,
nonce: res.locals.nonce
});

req.log.info("Jira configuration rendered successfully.");
Expand Down
9 changes: 5 additions & 4 deletions src/models/installation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import crypto from "crypto";
import Sequelize from "sequelize";
import Subscription from "./subscription";
import { Subscription } from "./index";
import SubscriptionClass from "./subscription";

// TODO: this should not be there. Should only check once a function is called
if (!process.env.STORAGE_SECRET) {
Expand Down Expand Up @@ -35,7 +36,7 @@ export default class Installation extends Sequelize.Model {
static async getForHost(host: string): Promise<Installation | null> {
return Installation.findOne( {
where: {
jiraHost: host,
jiraHost: host
},
order: [["id", "DESC"]]
});
Expand All @@ -44,7 +45,7 @@ export default class Installation extends Sequelize.Model {
static async getAllForHost(host: string): Promise<Installation[]> {
return Installation.findAll({
where: {
jiraHost: host,
jiraHost: host
},
order: [["id", "DESC"]]
});
Expand Down Expand Up @@ -94,7 +95,7 @@ export default class Installation extends Sequelize.Model {
await this.destroy();
}

async subscriptions(): Promise<Subscription[]> {
async subscriptions(): Promise<SubscriptionClass[]> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder what's the difference between Subscription and SubscriptionClass? And how does this type change affect things?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, we need to fix that at some point. It's because we're using an export default class called Subscription at src/models/subscription but we also have a export const value called Subscription which is an instance of the class that's being built by Sequelize and provided by src/models/index. Values and Class types are different and you cannot place a value as a type of a function. If you look at the imports at the top of the file, I had to import both the class type and the value as you need to refer to the value if you want to do operations on the DB with sequelize or else it'll just error at runtime.

return Subscription.getAllForClientKey(this.clientKey);
}
}
Expand Down
Loading