-
Notifications
You must be signed in to change notification settings - Fork 224
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
fix: pipeline diff fails on v10 #3154
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,9 +3,10 @@ import {Command, flags} from '@heroku-cli/command' | |
import {ux} from '@oclif/core' | ||
import HTTP from 'http-call' | ||
|
||
import {getCoupling, getReleases, listPipelineApps, SDK_HEADER} from '../../lib/api' | ||
import {getCoupling, getPipeline, getReleases, listPipelineApps, SDK_HEADER} from '../../lib/api' | ||
import KolkrabbiAPI from '../../lib/pipelines/kolkrabbi-api' | ||
import {OciImage, Slug} from '../../lib/types/fir' | ||
import type {OciImage, Slug, Pipeline, PipelineCoupling} from '../../lib/types/fir' | ||
import type {Commit, GitHubDiff} from '../../lib/types/github' | ||
|
||
interface AppInfo { | ||
name: string; | ||
|
@@ -35,19 +36,26 @@ async function diff(targetApp: AppInfo, downstreamApp: AppInfo, githubToken: str | |
// Do the actual GitHub diff | ||
try { | ||
const path = `${targetApp.repo}/compare/${downstreamApp.hash}...${targetApp.hash}` | ||
const headers: { authorization: string; 'user-agent'?: string} = {authorization: 'token ' + githubToken} | ||
const headers = { | ||
authorization: 'token ' + githubToken, | ||
'Content-Type': 'application/vnd.github+json', | ||
'X-GitHub-Api-Version': '2022-11-28', | ||
} | ||
|
||
if (herokuUserAgent) { | ||
headers['user-agent'] = herokuUserAgent | ||
Reflect.set(headers, 'user-agent', herokuUserAgent) | ||
} | ||
|
||
const githubDiff: any = await HTTP.get(`https://api.github.com/repos/${path}`, { | ||
headers, | ||
}).then(res => res.body) | ||
let githubDiff: GitHubDiff | ||
try { | ||
({body: githubDiff} = await HTTP.get<GitHubDiff>(`https://api.github.com/repos/${path}`, {headers})) | ||
} catch { | ||
throw new Error(`unable to perform diff for ${targetApp.name} and ${downstreamApp.name}. Are you sure you have pushed your latest commits to GitHub?`) | ||
} | ||
|
||
ux.log('') | ||
ux.styledHeader(`${color.app(targetApp.name)} is ahead of ${color.app(downstreamApp.name)} by ${githubDiff.ahead_by} commit${githubDiff.ahead_by === 1 ? '' : 's'}`) | ||
const mapped = githubDiff.commits.map((commit: any) => { | ||
const mapped = githubDiff.commits.map((commit: Commit) => { | ||
return { | ||
sha: commit.sha.slice(0, 7), | ||
date: commit.commit.author.date, | ||
|
@@ -64,7 +72,6 @@ async function diff(targetApp: AppInfo, downstreamApp: AppInfo, githubToken: str | |
message: {}, | ||
}) | ||
ux.log(`\nhttps://github.com/${path}`) | ||
// tslint:disable-next-line: no-unused | ||
} catch { | ||
ux.log(`\n${color.app(targetApp.name)} was not compared to ${color.app(downstreamApp.name)} because we were unable to perform a diff`) | ||
ux.log('are you sure you have pushed your latest commits to GitHub?') | ||
|
@@ -126,20 +133,27 @@ export default class PipelinesDiff extends Command { | |
const {flags} = await this.parse(PipelinesDiff) | ||
const targetAppName = flags.app | ||
|
||
const coupling = await getCoupling(this.heroku, targetAppName) | ||
.then(res => res.body) | ||
.catch(() => {}) | ||
|
||
if (!coupling) { | ||
let coupling: PipelineCoupling | undefined | ||
try { | ||
({body: coupling} = await getCoupling(this.heroku, targetAppName)) | ||
} catch { | ||
ux.error(`This app (${targetAppName}) does not seem to be a part of any pipeline`) | ||
return | ||
} | ||
|
||
const targetAppId = coupling.app.id! | ||
const generation = coupling.generation | ||
let pipeline: Pipeline | ||
try { | ||
({body: pipeline} = await getPipeline(this.heroku, coupling.pipeline!.id!)) | ||
} catch { | ||
ux.error(`Unable to fetch pipeline ${coupling.pipeline!.name}`) | ||
return | ||
} | ||
Comment on lines
+148
to
+150
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here as my previous comment. Just rely on the error returned by Platform API and don't introduce new error messages. This is different from the pipeline couplings get request in that we expect that request to fail under the scenario of the app not being part of a pipeline, but in this case, once the coupling was fetched this request isn't expected to fail, and the error returned from Platform API is more specific and probably more useful to the customer than a generic error message. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like there could be better consistency with the endpoints. How does a developer know when to use a custom error versus letting the error flow though from the API? When developing this fix, I put this error in intentionally because I didn't find the endpoint error particularly helpful. It was something like "Could not find the pipeline" or "pipeline not found" which wasn't helpful. |
||
|
||
const targetAppId = coupling!.app!.id! | ||
const generation = pipeline!.generation!.name! | ||
|
||
ux.action.start('Fetching apps from pipeline') | ||
const allApps = await listPipelineApps(this.heroku, coupling.pipeline.id!) | ||
const allApps = await listPipelineApps(this.heroku, coupling!.pipeline!.id!) | ||
ux.action.stop() | ||
|
||
const sourceStage = coupling.stage | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,8 +5,9 @@ import {ux} from '@oclif/core' | |
import {findIndex as lodashFindIndex} from 'lodash' | ||
import {Result} from 'true-myth' | ||
import push from '../git/push' | ||
import {OciImage, Release} from '../../lib/types/fir' | ||
import {OciImage} from '../../lib/types/fir' | ||
import {isURL} from 'validator' | ||
import * as Heroku from '@heroku-cli/schema' | ||
|
||
export type BuildpackResponse = { | ||
buildpack: { | ||
|
@@ -29,7 +30,7 @@ export class BuildpackCommand { | |
async fetch(app: string, isFirApp = false): Promise<any[]> { | ||
let buildpacks: any | ||
if (isFirApp) { | ||
const {body: releases} = await this.heroku.request<Release[]>(`/apps/${app}/releases`, { | ||
const {body: releases} = await this.heroku.request<Heroku.Release[]>(`/apps/${app}/releases`, { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why refactoring this back to the old schema types instead of the Fir ones? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fir schemas did not match the response from the server whereas the old schemas did. |
||
partial: true, | ||
headers: { | ||
Range: 'version ..; max=10, order=desc', | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would avoid introducing new error messages and continue to rely on the error being surfaced by the API client with any messages returned by Github for now. This can be moved into a follow up refactor PR that will require review by CX.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There would be no error other than a 404 by this response. Just a plain 404...no other info. Is that what we want?