Skip to content

HPCC-35979 feat(eclccserver) Add github app support to eclccserver#21189

Open
ghalliday wants to merge 1 commit intohpcc-systems:candidate-10.0.xfrom
ghalliday:issue35979
Open

HPCC-35979 feat(eclccserver) Add github app support to eclccserver#21189
ghalliday wants to merge 1 commit intohpcc-systems:candidate-10.0.xfrom
ghalliday:issue35979

Conversation

@ghalliday
Copy link
Copy Markdown
Member

@ghalliday ghalliday commented Apr 9, 2026 •
edited
Loading

Type of change:

  • This change is a bug fix (non-breaking change which fixes an issue).
  • This change is a new feature (non-breaking change which adds functionality).
  • This change improves the code (refactor or other change that does not change the functionality)
  • This change fixes warnings (the fix does not alter the functionality or the generated code)
  • This change is a breaking change (fix or feature that will cause existing behavior to change).
  • This change alters the query API (existing queries will have to be recompiled)

Checklist:

  • My code follows the code style of this project.
    • My code does not create any new warnings from compiler, build system, or lint.
  • The commit message is properly formatted and free of typos.
    • The commit message title makes sense in a changelog, by itself.
    • The commit is signed.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly, or...
    • I have created a JIRA ticket to update the documentation.
    • Any new interfaces or exported functions are appropriately commented.
  • I have read the CONTRIBUTORS document.
  • The change has been fully tested:
    • I have added tests to cover my changes.
    • All new and existing tests passed.
    • I have checked that this change does not introduce memory leaks.
    • I have used Valgrind or similar tools to check for potential issues.
  • I have given due consideration to all of the following potential concerns:
    • Scalability
    • Performance
    • Security
    • Thread-safety
    • Cloud-compatibility
    • Premature optimization
    • Existing deployed queries will not be broken
    • This change fixes the problem, not just the symptom
    • The target branch of this pull request is appropriate for such a change.
  • There are no similar instances of the same problem that should be addressed
    • I have addressed them here
    • I have raised JIRA issues to address them separately
  • This is a user interface / front-end modification
    • I have tested my changes in multiple modern browsers
    • The component(s) render as expected

Smoketest:

  • Send notifications about my Pull Request position in Smoketest queue.
  • Test my draft Pull Request.

Testing:

@ghalliday ghalliday requested a review from dcamper April 9, 2026 09:12
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 9, 2026

🔄 Upmerge Test Results

Status: ✅ All branches merged successfully
PR: #21189 - HPCC-35979 feat(eclccserver) Add github app support to eclccserver
Base Branch: candidate-10.0.x
Test Time: 2026-04-09 09:14:19 UTC

✅ Successful Branches (2)

  • candidate-10.2.x
  • master
    This comment was automatically generated by the upmerge test workflow.

dcamper
dcamper requested changes Apr 9, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@dcamper dcamper left a comment

Choose a reason for hiding this comment

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

A couple of minor comments.

Comment thread system/jlib/jsecrets.cpp
#ifdef verify
#define JWT_HAS_VERIFY_MACRO
#define OLD_VERIFY verify
#undef verify
Copy link
Copy Markdown
Contributor

@dcamper dcamper Apr 9, 2026

Choose a reason for hiding this comment

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

Should you reinstate the old verify? The code at system/security/plugins/jwtSecurity/jwtSecurity.cpp does, near the end:

// Reinstate the old macro value
#ifdef JWT_HAS_VERIFY_MACRO
    #define verify OLD_VERIFY
    #undef OLD_VERIFY
    #undef JWT_HAS_VERIFY_MACRO
#endif

I think it is only strictly necessary if any code within this file needs the original value of verify, but I wonder if it should be reinstated after all JWT calls simply in order to avoid invisible tech debt.

Copy link
Copy Markdown
Member Author

@ghalliday ghalliday Apr 10, 2026

Choose a reason for hiding this comment

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

I think copilot cloned this - I will update

Comment thread ecl/hql/hqlrepository.hpp Outdated
IEclRepository * createRepository(IEclSourceCollection * source, const char * rootScopeFullName, bool includeInArchive);

unsigned runGitCommand(StringBuffer * output, const char *args, const char * cwd, bool needCredentials);
unsigned runGitCommand(StringBuffer *output, const char *args, const char *cwd, bool needCredentials);
Copy link
Copy Markdown
Contributor

@dcamper dcamper Apr 9, 2026

Choose a reason for hiding this comment

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

Whitespace-only change that needs reverting to match the style in the rest of the file.

@ghalliday ghalliday requested a review from dcamper April 10, 2026 14:39
dcamper
dcamper approved these changes Apr 10, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@dcamper dcamper left a comment

Choose a reason for hiding this comment

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

One comment/opinion, but looks good.

Comment thread system/jlib/jsecrets.cpp

#ifdef JWT_HAS_VERIFY_MACRO
//Force an error if verify macro is used in any of the following code
#undef verify
Copy link
Copy Markdown
Contributor

@dcamper dcamper Apr 10, 2026

Choose a reason for hiding this comment

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

This may be just a matter of opinion, but disabling the macro like this is a little odd. I don't think verify is used much at all in the platform, but I also don't think it is outright undefined anywhere else so this becomes potentially new behavior during compilation.

@ghalliday
HPCC-35979 feat(eclccserver) Add github app support to eclccserver
49fb4b8 
Signed-off-by: Gavin Halliday <gavin.halliday@lexisnexis.com>
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 10, 2026

🔄 Upmerge Test Results

Status: ✅ All branches merged successfully
PR: #21189 - HPCC-35979 feat(eclccserver) Add github app support to eclccserver
Base Branch: candidate-10.0.x
Test Time: 2026-04-10 18:40:18 UTC

✅ Successful Branches (2)

  • candidate-10.2.x
  • master
    This comment was automatically generated by the upmerge test workflow.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 10, 2026

🔄 Upmerge Test Results

Status: ✅ All branches merged successfully
PR: #21189 - HPCC-35979 feat(eclccserver) Add github app support to eclccserver
Base Branch: candidate-10.0.x
Test Time: 2026-04-10 19:43:00 UTC

✅ Successful Branches (2)

  • candidate-10.2.x
  • master
    This comment was automatically generated by the upmerge test workflow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dcamper dcamper dcamper approved these changes

Assignees

No one assigned

Labels

Feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ghalliday @dcamper