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

feat: Reduce code duplicacy in chaos-workflow package #4010

Merged

Conversation

SohamRatnaparkhi
Copy link
Contributor

Proposed changes

Solves issue #4008

This PR adds 2 helper functions:

  1. updateManifestLabels - To set the manifest labels
  2. generateWeightsFromManifest - To generate new weights based on manifest labels.

Earlier the code in these functions was separately repeated at various instances. To avoid code repetition, this PR involves creating separate helper functions for the repeated code and calling these functions at appropriate instances.

Types of changes

What types of changes does your code introduce to Litmus? Put an x in the boxes that apply

  • New feature (non-breaking change which adds functionality)
  • Bugfix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices applies)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I have read the CONTRIBUTING doc
  • I have signed the commit for DCO to be passed.
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works (if appropriate)
  • I have added necessary documentation (if appropriate)

Dependency

  • Please add the links to the dependent PR need to be merged before this (if any).

@namkyu1999
Copy link
Member

Please check DCO!

@SohamRatnaparkhi SohamRatnaparkhi force-pushed the feature/gql_code_duplicacy branch 2 times, most recently from 1dea1d6 to 772add9 Compare June 20, 2023 13:20
@namkyu1999 namkyu1999 added LFX-MENTORSHIP Linux Foundation Mentor ship Issue go Pull requests that update Go code READY-FOR-REVIEW labels Jun 20, 2023
@SohamRatnaparkhi
Copy link
Contributor Author

Please check DCO!

Rebased the commits and signed them 👍

Copy link
Contributor

@Saranya-jena Saranya-jena left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

litmus-portal/graphql-server/pkg/chaos-workflow/service.go Outdated Show resolved Hide resolved
cronWorkflowManifest.Labels["cluster_id"] = workflow.ClusterID
cronWorkflowManifest.Labels["workflows.argoproj.io/controller-instanceid"] = workflow.ClusterID
}
cronWorkflowManifest.Labels = updateManifestLabels(cronWorkflowManifest.Labels, *workflow.WorkflowID, workflow.ClusterID, false)

if cronWorkflowManifest.Spec.WorkflowMetadata == nil {
Copy link
Member

Choose a reason for hiding this comment

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

the if block should also be using the same updateLabels function or else if logic changes in the function or here it will cause a discrepancy in the labels.

@SohamRatnaparkhi SohamRatnaparkhi marked this pull request as draft June 30, 2023 20:32
SohamRatnaparkhi and others added 3 commits July 1, 2023 02:02
Signed-off-by: SohamRatnaparkhi <[email protected]>
Co-authored-by: Amit Kumar Das <[email protected]>
Signed-off-by: SohamRatnaparkhi <[email protected]>
@SohamRatnaparkhi SohamRatnaparkhi marked this pull request as ready for review June 30, 2023 21:00
Copy link
Member

@gdsoumya gdsoumya left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@imrajdas imrajdas merged commit fdf5113 into litmuschaos:master Jul 3, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go Pull requests that update Go code LFX-MENTORSHIP Linux Foundation Mentor ship Issue READY-FOR-REVIEW
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants