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

fix: [#SRE-1865] clicop tries to check all clickup-like tickets, but considers only valid ones for the CI check #7

Merged
merged 2 commits into from
May 31, 2024

Conversation

rampsawer
Copy link
Contributor

@rampsawer rampsawer commented May 28, 2024

Changes

  • axios error handling only displays failed request url instead of full error, it compacts the error message, and clearly shows which request (ticketid) failed
  • only validated tickets array (tasksWithName) is considered for the CI check, instead of everything that looks like a ticket (tasks array)

QA

  • create a draft PR in ramp-instant, go to .github/workflows/clicop.yml, change clicop job to use this branch
    - uses: RampNetwork/clicop@fix/clicop
  • add to PR description invalid clickup id's
  • add to PR description valid clickup id's

Closes: #SRE-1865

Copy link
Contributor

@prgres prgres left a comment

Choose a reason for hiding this comment

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

crossover episode

Copy link
Contributor

@kwaszczuk kwaszczuk left a comment

Choose a reason for hiding this comment

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

Just a two minor nitpicks, besides that LGTM

Comment on lines 70 to 73
const parallelRequests = (tasks = [], req) => {
if (tasks.length === 0) {
return;
return[];
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I believe this check can be removed. If tasks is empty the Promise.all(...) should return an empty array anyway.

resource,
method: 'GET',
});
return name
return response?.data?.name ?? undefined
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
return response?.data?.name ?? undefined
return response?.data?.name

nit: ?? is redundant. ?. optional chaining will already return undefined if any property is missing.

@rampsawer rampsawer merged commit 448f075 into main May 31, 2024
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.

3 participants