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 Failing Build Probe #422

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

AayushSaini101
Copy link
Contributor

@AayushSaini101 AayushSaini101 commented Dec 25, 2023

Description

Closes #363

Add a new probe to check whether the build failed on the the default branch or not.

Testing done

Added new test cases for testing the probe.

Submitter checklist

  • If an issue exists, it is well described and linked in the description

Comment on lines 24 to 32
@Test
void shouldCorrectlyDetectMissingJenkinsfile() throws IOException {
final FailingBuildProbe probe = getSpy();
final Plugin plugin = mock(Plugin.class);
final ProbeContext ctx = mock(ProbeContext.class);
final Path repo = Files.createTempDirectory("foo");
when(ctx.getScmRepository()).thenReturn(Optional.of(repo));
assertThat(probe.apply(plugin, ctx)).withFailMessage("No JenkinsFile found");
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is already taking care off by the Jenkinsfile probe.

What I think would be beneficial for this pull request is to validate the behaviour when we cannot get the run of the latest commit of the default branch, to make sure which "run" you are getting with the iterator().next(), if it's possible for the conclusion to be null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @alecharp Is the above logic to check the status of the default branch in the probe is not correct ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, @alecharp

@Order(FailingBuildProbe.ORDER)
public class FailingBuildProbe extends Probe {

public static final int ORDER = LastCommitDateProbe.ORDER + 100;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Run it after the Jenkinsfile probe and you can use its result to make sure there is a Jenkinsfile present.

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 @alecharp

Comment on lines 80 to 90
public ProbeResult repoContainsJenkins(ProbeContext context) {
final Path repository = context.getScmRepository().get();
try (Stream<Path> paths = Files.find(repository, 1, (file, $) ->
Files.isReadable(file) && "JenkinsFile".equals(file.getFileName().toString()))) {
return paths.findFirst()
.map(file -> this.success("Jenkinsfile found"))
.orElseGet(() -> this.error("No Jenkinsfile found"));
} catch (IOException e) {
return this.error(e.getMessage());
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is doing the Jenkinsfile probe job again. Not sure this is necessary.

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 @alecharp Removed and update the order

Comment on lines 63 to 71
GHCommit commit = repository.getCommit(defaultBranch);
GHCheckRun checkRun = commit.getCheckRuns().iterator().next();
GHCheckRun.Conclusion conclusion = checkRun.getConclusion();

if (conclusion == GHCheckRun.Conclusion.FAILURE) {
return this.success("Build Failed in Default Branch");
} else {
return this.success("Build is Success in Default Branch");
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Those statements need to be tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alecharp can you help me in the testing, GHCommit ghCommit = new GHCommit(); i cannot set particular commits for testing. any kind of more generic way to test it

@alecharp alecharp added the enhancement New feature or request label Jan 8, 2024
@alecharp
Copy link
Collaborator

alecharp commented Feb 5, 2024

@AayushSaini101 Are you still interesting to drive this pull request?

@AayushSaini101
Copy link
Contributor Author

@AayushSaini101 Are you still interesting to drive this pull request?

Yes, @alecharp I can work on this. Will submit the changes by ASAP

@AayushSaini101 AayushSaini101 marked this pull request as draft February 5, 2024 16:05
@AayushSaini101
Copy link
Contributor Author

Closing this @alecharp due to multiple accounts. Creating new one

@alecharp
Copy link
Collaborator

alecharp commented Feb 5, 2024

Please, stop doing that. Use the proper account from the start. You are wasting the review the discussion already started here.

@AayushSaini101
Copy link
Contributor Author

AayushSaini101 commented Feb 5, 2024

Please, stop doing that. Use the proper account from the start. You are wasting the review the discussion already started here.

Sorry, This was my last PR that contained issue due to multiple accounts, I have resolved the issues in the remaining ones.

@AayushSaini101 AayushSaini101 reopened this Feb 5, 2024
@alecharp
Copy link
Collaborator

@AayushSaini101 have you plan to attend this pull request?

@AayushSaini101
Copy link
Contributor Author

@AayushSaini101 have you plan to attend this pull request?

Sorry @alecharp for being late for this PR, I am working on the GSOC proposal for this project, I will try to complete by this week.

@AayushSaini101 AayushSaini101 marked this pull request as ready for review February 21, 2024 16:34
@AayushSaini101
Copy link
Contributor Author

@alecharp Can you help me to write test case for the changes, I am not sure how can i test the changes properly

Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing the license as well

@alecharp
Copy link
Collaborator

Please, do not force push branch on pull request that has already been reviewed.
Some of the commits were made with different user as well.

There is still no tests to validate the probe.

@AayushSaini101
Copy link
Contributor Author

Please, do not force push branch on pull request that has already been reviewed. Some of the commits were made with different user as well.

There is still no tests to validate the probe.

@alecharp I tried to push normally but cannot able to push even rebase and resolving the conflicts, there were multiple accounts in the last commits, may be the reason of this, I have tried many approaches to push normally but cannot, Sorry for this

@AayushSaini101
Copy link
Contributor Author

After removing the multiple accounts commits, now i can able to push normally

@AayushSaini101 AayushSaini101 marked this pull request as ready for review March 23, 2024 07:50
Copy link
Collaborator

@alecharp alecharp left a comment

Choose a reason for hiding this comment

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

I'm not sure you are convinced that what you did is correct.
I'll wait for you to fix the test class before reviewing it again.

Conclusion conclusion = checkRun.getConclusion();

if (conclusion == Conclusion.FAILURE) {
return this.success("Build Failed in Default Branch");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return this.success("Build Failed in Default Branch");
return this.success("Build failed in default branch");

if (conclusion == Conclusion.FAILURE) {
return this.success("Build Failed in Default Branch");
} else {
return this.success("Build is Success in Default Branch");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return this.success("Build is Success in Default Branch");
return this.success("Build is successful in default branch");


final Optional<String> repositoryName = context.getRepositoryName();
final GHRepository ghRepository = context.getGitHub().getRepository(repositoryName.get());
GHCommit commit = ghRepository.getCommit(ucPlugin.defaultBranch());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Value already assigned to a variable, let's use it.

Suggested change
GHCommit commit = ghRepository.getCommit(ucPlugin.defaultBranch());
GHCommit commit = ghRepository.getCommit(defaultBranch);

} else {
return this.success("Build is Success in Default Branch");
}
} catch (Exception ex) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we catch Exception, it means to catch anything that could go wrong. And we wouldn't know what went wrong.
Please, be more specific than Exception and log the exception.

@Order(DefaultBranchBuildStatusProbe.ORDER)
public class DefaultBranchBuildStatusProbe extends Probe {

public static final int ORDER = JenkinsCoreProbe.ORDER + 100;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to be executed after that probe?

Comment on lines +61 to +65
@BeforeEach
public void init() {

probe = getSpy();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to have the probe in a field?

final Plugin plugin = mock(Plugin.class);
final ProbeContext ctx = mock(ProbeContext.class);

final GitHub gh = mock(GitHub.class);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Already mocked on line 74.

Suggested change
final GitHub gh = mock(GitHub.class);

List.of()));
when(ctx.getGitHub()).thenReturn(gh);
when(ctx.getRepositoryName()).thenReturn(Optional.of(pluginRepo));
when(ctx.getGitHub()).thenReturn(gitHub);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alecharp I didn't get you regarding this

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I meant useless. you already mock that class before.

@alecharp alecharp marked this pull request as draft April 2, 2024 09:30
@alecharp
Copy link
Collaborator

@AayushSaini101 do you think you will have time to complete this pull request?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a probe for failing builds on the default branch
2 participants