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

CAMEL-21256: camel-github startingSha=last does not work with large history #15678

Merged

Conversation

electricsam
Copy link
Contributor

Description

I encountered an issue using startingSha=last with a repository with a large history. The last sha is not correctly resolved. CAMEL-17473 did not resolve this issue.

The following issues were discovered from real-world testing and a unit test:

  1. it appears that the first commit returned from the GitHub API is the most recent and the sha from that should be used as the lastSha variable.
  2. There is a threading issue where CommitConsumer.poll() can be called concurrently with CommitConsumer.doStart(). In some situations lastSha can be null in the poll() method, producing the same behavior as startingSha=beginning, which is undesired.

Target

  • I checked that the commit is targeting the correct branch (note that Camel 3 uses camel-3.x, whereas Camel 4 uses the main branch)

Tracking

  • If this is a large change, bug fix, or code improvement, I checked there is a JIRA issue filed for the change (usually before you start working on it).

Apache Camel coding standards and style

  • I checked that each commit in the pull request has a meaningful subject line and body.
  • I have run mvn clean install -DskipTests locally from root folder and I have committed all auto-generated changes.

Copy link
Contributor

🌟 Thank you for your contribution to the Apache Camel project! 🌟

🤖 CI automation will test this PR automatically.

🐫 Apache Camel Committers, please review the following items:

  • First-time contributors require MANUAL approval for the GitHub Actions to run

  • You can use the command /component-test (camel-)component-name1 (camel-)component-name2.. to request a test from the test bot.

  • You can label PRs using build-all, build-dependents, skip-tests and test-dependents to fine-tune the checks executed by this PR.

  • Build and test logs are available in the Summary page. Only Apache Camel committers have access to the summary.

  • ⚠️ Be careful when sharing logs. Review their contents before sharing them publicly.

@davsclaus
Copy link
Contributor

Can you update the PR as there are formatting changes

modified:   components/camel-github/src/main/java/org/apache/camel/component/github/consumer/CommitConsumer.java
modified:   components/camel-github/src/test/java/org/apache/camel/component/github/consumer/CommitConsumerLastTest.java

@electricsam
Copy link
Contributor Author

electricsam commented Sep 25, 2024

Can you update the PR as there are formatting changes

modified:   components/camel-github/src/main/java/org/apache/camel/component/github/consumer/CommitConsumer.java
modified:   components/camel-github/src/test/java/org/apache/camel/component/github/consumer/CommitConsumerLastTest.java

I aligned the CommitConsumer constructor arguments on line 49. That was the only unnecessary formatting change that I saw. Was there anything else?

The other changes I made in my initial commit involved moving the existing code over 4 spaced to be inside a synchronized block.

@apupier
Copy link
Contributor

apupier commented Sep 26, 2024

/component-test github

Result ✅ The tests passed successfully

Copy link
Contributor

🤖 The Apache Camel test robot will run the tests for you 👍

@davsclaus davsclaus force-pushed the CAMEL-21256-camel-github-starting-sha-issue branch from 13d7f82 to f74d197 Compare September 26, 2024 13:36
@davsclaus davsclaus merged commit f60712f into apache:main Sep 26, 2024
4 checks passed
davsclaus pushed a commit that referenced this pull request Sep 26, 2024
…istory (#15678)

* CAMEL-21256: use most recent sha when startingSha=last, prevent CommitConsumer.poll() logic from executing until after CommitConsume.doStart() method completes.

* CAMEL-21256: fix formatting of CommitConsumer constructor arguments

* CAMEL-21256: fix formatting on line 87 of CommitConsumer

* CAMEL-21256: remove extra empty line from CommitConsumerLastTest
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants