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

Regression Fix + Refine Status Updates + Resolve Edge Cases + Code Cleanup #193

Merged
merged 75 commits into from
Aug 9, 2024

Conversation

sakshibobade21
Copy link
Contributor

@sakshibobade21 sakshibobade21 commented Jun 18, 2024

Proposed changes

This PR addresses Issue:
This PR resolves a regression issue where the status for the VSAM stage was being updated without the stage being completed. It also addresses edge cases where the status was not updated as expected when stages were skipped or completed. The implementation now ensures that the status of stages and substages is properly stored and initialized accordingly. Additionally, this PR includes some code cleanup

This PR depends upon the following PRs:

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Change in a documentation
  • Refactor the code
  • Chore, repository cleanup, updates the dependencies.
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

PR Checklist

Please delete options that are not relevant.

  • If the changes in this PR are meant for the next release / mainline, this PR targets a "staging" branch.
  • My code follows the style guidelines of this project (see: Contributing guideline)
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • New and existing unit tests pass locally with my changes
  • video or image is included if visual changes are made
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works, or describe a test method below

Testing

Further comments

@sakshibobade21 sakshibobade21 changed the title Regression - vsam status [WIP] Refine Status Updates + Resolve Edge Case Regressions Jun 24, 2024
@sakshibobade21 sakshibobade21 changed the title [WIP] Refine Status Updates + Resolve Edge Case Regressions [WIP] Regression Fix + Refine Status Updates + Resolve Edge Cases Jun 24, 2024
@sakshibobade21 sakshibobade21 changed the title [WIP] Regression Fix + Refine Status Updates + Resolve Edge Cases [WIP] Regression Fix + Refine Status Updates + Resolve Edge Cases + Code Cleanup Jun 26, 2024
@sakshibobade21 sakshibobade21 changed the title [WIP] Regression Fix + Refine Status Updates + Resolve Edge Cases + Code Cleanup Regression Fix + Refine Status Updates + Resolve Edge Cases + Code Cleanup Jun 26, 2024
skipStatus.reviewInstallation
]

return skipStatusArray[subStageId-1];
Copy link
Contributor

Choose a reason for hiding this comment

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

check if array index out of bounds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks

src/renderer/components/common/Stepper.tsx Outdated Show resolved Hide resolved
@sakshibobade21 sakshibobade21 changed the title Regression Fix + Refine Status Updates + Resolve Edge Cases + Code Cleanup [Do not merge b4 Zen 1st Release]Regression Fix + Refine Status Updates + Resolve Edge Cases + Code Cleanup Jul 11, 2024
@sakshibobade21 sakshibobade21 changed the title [Do not merge b4 Zen 1st Release]Regression Fix + Refine Status Updates + Resolve Edge Cases + Code Cleanup Regression Fix + Refine Status Updates + Resolve Edge Cases + Code Cleanup Jul 15, 2024
Copy link
Collaborator

@skurnevich skurnevich left a comment

Choose a reason for hiding this comment

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

So in the current state we do not distinguish between failed and skipped steps?

Also, for instance if Certificates step fails - it shows job output as error, but then we skip it and go to vsam (which should be completely optional) and it fails too - it will show error in the header, and the job log stays from the previous failure in Certificates. It looks inconsistent and misleading.

I do not mind merging this though, as it doesn't break things and in order to continue the development process.

@sakshibobade21
Copy link
Contributor Author

@skurnevich Thank you for reviewing the PR.

At present, in ZEN, we do not differentiate between failed and skipped steps. This could be a potential enhancement to consider for future improvements.

Regarding your comment about the certificate stage, I think we should address that in a separate PR since it originates from the staging. I will create a ticket for this.

Thanls.

@skurnevich
Copy link
Collaborator

Hi Sakshi, that is fine with me, please resolve Lenny's changes request and we can merge this PR

@sakshibobade21
Copy link
Contributor Author

sakshibobade21 commented Aug 6, 2024

@skurnevich thanks. And the changes requested by Lenny have already been addressed.

Also, I do not have the merge rights for this repository. Could you please merge it if you are ok with it? This PR will serve as a foundation for future PRs in ZEN.

@skurnevich
Copy link
Collaborator

Hi @sakshibobade21 , i'll merge this PR, but problem is the github still has "Changes requested" as blocker for merge there.
Screenshot 2024-08-06 at 11 11 15
As far as i can see the requests were addressed, and I can merge it bypassing the protection if @DivergentEuropeans does not mind.
But the proper way to resolve this is to re-request review,
Screenshot 2024-08-06 at 11 25 24
Github sometimes acts weird with permissions, so can you please check if you have this option in the top right corner first?

@sakshibobade21
Copy link
Contributor Author

sakshibobade21 commented Aug 6, 2024

@skurnevich I re-requested the review now. Hope that works.
Thank you.

@sakshibobade21 sakshibobade21 mentioned this pull request Aug 7, 2024
14 tasks
@DivergentEuropeans
Copy link
Member

Thanks, dismissed stale review @sakshibobade21 @skurnevich

@skurnevich skurnevich merged commit 39b668d into v2.x/staging Aug 9, 2024
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

4 participants