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: [CI-14390]: Added new enhancements to plugins/jira #26

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

vanisrikanithi
Copy link

@vanisrikanithi vanisrikanithi commented Sep 25, 2024

This review covers all Jira issues #21,#22,#23,#24 as per jira spec.
There are some commented lines which are used for debugging. once the review is approved, will remove them.

@vanisrikanithi vanisrikanithi changed the title JEN-1 commit Jira Issues changes [JIRA] JEN-1 Jira plugin support for the Issues Sep 26, 2024
Copy link
Member

@Ompragash Ompragash left a comment

Choose a reason for hiding this comment

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

also, add unit tests for the changes in plugin_test.go and util_test.go respectively

plugin/plugin.go Outdated
State: state,
UpdateSequenceNumber: args.Build.Number,
References: []struct {
Copy link
Member

Choose a reason for hiding this comment

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

add a check to make sure the branch and URI values exist before adding them to the References so it'll only be added when the necessary values are available

plugin/plugin.go Outdated
Comment on lines 104 to 113
if len(args.IssueKeys) > 0 {
issues = args.IssueKeys
} else {
issue = extractIssue(args)
if issue == "" {
logger.Debugln("cannot find issue number")
return errors.New("failed to extract issue number")
}
logger = logger.WithField("issue", issue)
}
Copy link
Member

Choose a reason for hiding this comment

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

let's not duplicate how we handle both the array (issues) and the single issue (issue [old logic]) separately, and keep the logic simple:

// check if PLUGIN_ISSUEKEYS is provided
if len(args.IssueKeys) > 0 {
issues = args.IssueKeys
} else {
// fallback to extracting from commit if no issue keys are passed
issue = extractIssue(args)
if issue == "" {
logger.Debugln("cannot find issue number")
return errors.New("failed to extract issue number")
}
issues = []string{issue} // add the single issue here for consistency
}

and modify the deployment payload accordingly.

Copy link
Author

@vanisrikanithi vanisrikanithi Sep 26, 2024

Choose a reason for hiding this comment

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

Initially I have done this way, but observed one issue that if PLUGIN_ISSUEKEYS are not provided then Issue keys will be populated by old way(extractIssue) right. That means deployment payload contains always both Issuekeys and associations like below. With this payload when i tested restapi (postman) I was getting error in the response as below ("message": "issueKeys and associations are mutually exclusive.) Is this expected.? Also it could be existing behaviour. It won't cause any issue?
Request chunk:
"issueKeys": [
"SCRUM-1"
],
"associations": [
{
"associationType": "issueIdOrKeys",
"values": [
"SCRUM-1"
]
}
],

Response chunk:
"errors": [
{
"message": "issueKeys and associations are mutually exclusive. Either only specify issueKeys or pass issueKeys as an associationType.",
"key": "deploymentdata.issueKeysOrAssociationsOrNone.invalid"
},

plugin/util.go Outdated Show resolved Hide resolved
plugin/util.go Show resolved Hide resolved
@Ompragash Ompragash changed the title [JIRA] JEN-1 Jira plugin support for the Issues feat: [CI-14390]: Added new enhancements to plugins/jira Sep 26, 2024
@vanisrikanithi
Copy link
Author

also, add unit tests for the changes in plugin_test.go and util_test.go respectively

util_test.go updated with unit tests for new functions. In plugin.go not implemented any new

also, add unit tests for the changes in plugin_test.go and util_test.go respectively
Added test cases for all the new apis that are implemented in util_test.go. There are no new APIs are implemented in plugin.go

@vanisrikanithi
Copy link
Author

Review comments addressed and updated the new patch.

plugin/plugin.go Outdated
if len(args.IssueKeys) > 0 {
deploymentPayload.Deployments[0].Associations = nil
}
/*fmt.Println("formatted deploymentPayload JSON data")

Choose a reason for hiding this comment

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

remove this comment please. Please make sure those debug/temp/commented out code doesn't appear in final PR

Copy link
Author

Choose a reason for hiding this comment

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

done

plugin/util.go Outdated
}
} else {
// Log the error if URL parsing fails
log.Printf("Error parsing URL '%s': %v", instance, err)
Copy link
Member

Choose a reason for hiding this comment

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

consider using the logger like logrus to maintain consistency across the plugin

Copy link
Author

Choose a reason for hiding this comment

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

done

@vanisrikanithi
Copy link
Author

@Ompragash @jamie-harness I have addressed the review comments. Please take a look at the latest changes.

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.

4 participants