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

Refactor and clean up the createStagingDeployIssue code #9999

Closed
mountiny opened this issue Jul 19, 2022 · 9 comments
Closed

Refactor and clean up the createStagingDeployIssue code #9999

mountiny opened this issue Jul 19, 2022 · 9 comments
Assignees
Labels
Engineering Internal Requires API changes or must be handled by Expensify staff Monthly KSv2 NewFeature Something to build that is a new item.

Comments

@mountiny
Copy link
Contributor

mountiny commented Jul 19, 2022

Problem

Coming from #9946 (comment)

The code for creating staging deploy issue is very hard to follow and leads to bugs when new features are added or code is changed as engineers have tough time following the code.

Also sometimes we can manually move PRs to be internalQA after being added to the checklist - try to check if the PRs in main section are indeed internal or not before adding them in - do not rely on previous checklist in terms of this.

Why is this important?

Intoruduced more bugs in the issues which slows down deploy and takes more time to fix.

Solution

We should do:

  • clean up and refactor the code/flow so it is easier to follow
  • make sure to add more comments which will clearly indicate what are the formats/structures of the arrays/objects and in general comments explaining the flow
  • Find a nicer way to get the URL from the internal QA section than this
    url: match[2].split('-')[0].trim(),

And:

Also sometimes we can manually move PRs to be internalQA after being added to the checklist - try to check if the PRs in main section are indeed internal or not before adding them in - do not rely on previous checklist in terms of this.

Lets also update naming to make sure it is not outdated:

replace StagingDeployCash with DeployChecklist, and LockCashDeploys with LockDeploys

cc @roryabraham

@mountiny
Copy link
Contributor Author

Low priority for now

@mountiny
Copy link
Contributor Author

Focusing on API refactors and n7

@melvin-bot melvin-bot bot added the Overdue label Oct 17, 2022
@mountiny
Copy link
Contributor Author

Still in my backlog, I will let this once to the pool if anyone is interested

@melvin-bot melvin-bot bot removed the Overdue label Oct 17, 2022
@mountiny mountiny removed their assignment Oct 17, 2022
@roryabraham
Copy link
Contributor

Overall I think we may have lost the context for this issue and the problem is unclear. I think we should refine the problem statement or close this out.

@mountiny
Copy link
Contributor Author

@roryabraham I think the general goal was to mrefactor the entire code to me more accessible to unfamiliar developers. A bit vague goal I know.

However, coming back to what you mentioned before offshore, about reworking the deploy process, is there anything you planned for the deploy checklist and how we create it? then we might just close this out and focus on this later

@JmillsExpensify JmillsExpensify added the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Oct 18, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 18, 2022

Triggered auto assignment to @puneetlath (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@melvin-bot melvin-bot bot added Daily KSv2 and removed AutoAssignerTriage Auto assign issues for triage to an available triage team member Monthly KSv2 labels Oct 18, 2022
@puneetlath puneetlath added Monthly KSv2 NewFeature Something to build that is a new item. and removed Daily KSv2 Improvement Item broken or needs improvement. labels Oct 18, 2022
@puneetlath puneetlath removed their assignment Oct 18, 2022
@puneetlath puneetlath added the Internal Requires API changes or must be handled by Expensify staff label Oct 18, 2022
@roryabraham
Copy link
Contributor

I didn't really have anything major planned for the checklist other than removing some outdated naming (i.e: replace StagingDeployCash with DeployChecklist, and LockCashDeploys with LockDeploys)

@mountiny mountiny self-assigned this Oct 19, 2022
@mountiny
Copy link
Contributor Author

Alright, I think from the bugZero perspective, we can close this one out. The issue got created when we were trying to add a new feature to the checklist and it revealed how easy it is to miss something in the complicated code. However, we havent had issues with the checklist for a long time or needs to change it so I think we could close this now and if there would be any new bigger changes required for the checklist in future, the refactoring should take place along with it.

Let me know if you disagree.

@puneetlath
Copy link
Contributor

Sounds good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering Internal Requires API changes or must be handled by Expensify staff Monthly KSv2 NewFeature Something to build that is a new item.
Projects
None yet
Development

No branches or pull requests

4 participants