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

Add projects and components for validation into the config file #48

Closed
wants to merge 2 commits into from

Conversation

xjusko
Copy link
Collaborator

@xjusko xjusko commented Jan 30, 2024

resolves #30

private void validateParticipantProjects() throws ConfigException {
for (Participant participant : participants) {
for (Project project : participant.projects) {
if (!availableProjects.containsKey(project.project)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should do case-insensitive string comparison. I believe JQL is case insensitive as well

}

Set<String> availableComponents = availableProjects.get(project.project);
if (!availableComponents.containsAll(project.components)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here


public record Participation(@JsonProperty(required = true) int maxIssues) {
public static class ConfigException extends Exception {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure, if we need a custom exception here.

public Project(@JsonProperty(required = true) String project, @JsonProperty(required = true) Set<String> components,
@JsonProperty(required = true) int maxIssues) {
this(project, components, new Participation(maxIssues));
public void validate() throws ConfigException {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would probably keep the config just for config and validation in a different file or directly in LotteryConfigProducer could have this method as private.

@@ -1,3 +1,3 @@
camel.component.jira.jira-url=https://issues.redhat.com/
%test.jira-issue-lottery.access-token=ignored
jira-issue-lottery.config-file-repo=https://github.com/The-Huginn/jira-issue-lottery
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure about this, perhaps each of us should keep this in .env file and mention this in readme

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should I create a new issue for that? Then I can add a new commit to this PR and mention both issues.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess so, but there is that one test I commented below, that would need to be adjusted as well, keep that in mind then. However, I do not think there are any particular test fetching a config file anyways, so might be some arbitrary value, for example jboss-set even if the file doesn't exist here.

@@ -222,7 +252,8 @@ public void testAppendingRepositoryForUnsubcription() throws Exception {
List<Mail> sent = mailbox.getMailsSentTo(email);
assertEquals(1, sent.size());
// Check directly for this URL as we know it is valid one. Using templates might result in a corrupted URL overall
assertTrue(sent.get(0).getText().contains("https://github.com/The-Huginn/jira-issue-lottery/blob/main/.github/jira-issue-lottery.yml"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

this doesn't seem right, as this is a single test with concrete value, with the intention of making sure a valid link is generated

@xjusko
Copy link
Collaborator Author

xjusko commented Feb 22, 2024

resolves #49

@xjusko xjusko force-pushed the config-project-validation branch 2 times, most recently from 13b9c51 to 7ac7fd0 Compare February 26, 2024 09:48
…rties to .env file and set a default for testing.
@The-Huginn
Copy link
Collaborator

Thanks for your help @xjusko . After some conversations we have decided to move forward with a different approach.

@The-Huginn The-Huginn closed this Feb 27, 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.

Add projects and components for validation into the config file
2 participants