[DRAFT] Update DFIU to verify whether renovate is installed #1133
Conversation
AvvariSaiBharadwaj
requested review from
afalko,
avimanyum and
jeetchoudhary
as code owners
|
Thanks for the contribution! It looks like @saiavvari is an internal user so signing the CLA is not required. However, we need to confirm this. |
| @@ -42,6 +42,8 @@ private Constants() { | |||
| public static final String IGNORE_IMAGE_STRING = "x"; | |||
| public static final String FILE_NAMES_TO_SEARCH = "filenamestosearch"; | |||
| public static final String RATE_LIMIT_PR_CREATION = "rate_limit_pr_creations"; | |||
| public static final String RENOVATE_GITHUB_APP_ID = "appId"; | |||
There was a problem hiding this comment.
This should be more generic than renovate. The logic is to skip DFIU if a particular github app is installed
There was a problem hiding this comment.
100%. WIll refactor the entire code to make it more generic.
| import java.time.Instant; | ||
| import java.util.Date; | ||
|
|
||
| public class RenovateUtil { |
There was a problem hiding this comment.
| public class RenovateUtil { | |
| public class GithubAppCheck { |
Util is not a good descriptive name
| } | ||
| } | ||
|
|
||
| protected boolean isRenovateEnabledOnRepository(String fullRepoName){ |
There was a problem hiding this comment.
let's make this more generic - to check for any app existance
| public class RenovateUtil { | ||
| private static final Logger log = LoggerFactory.getLogger(RenovateUtil.class); | ||
|
|
||
| private static String appId; |
There was a problem hiding this comment.
these all need to be made final static is not a good practice
AvvariSaiBharadwaj
changed the title
| this.appId = ns.get(Constants.RENOVATE_GITHUB_APP_ID); | ||
| this.privateKeyPath = ns.get(Constants.RENOVATE_GITHUB_APP_KEY); |
There was a problem hiding this comment.
Why do we have to do ns.get() in this case instead of calling directly from Constants?
There was a problem hiding this comment.
This is to call the CLI input to the Java code. The constant is the CLI identifier for the input.
| protected boolean isRenovateEnabledOnRepository(String fullRepoName){ | ||
| refreshJwtIfNeeded(appId, privateKeyPath); | ||
| try { | ||
| gitHub.getApp().getInstallationByRepository(fullRepoName.split("/")[0], fullRepoName.split("/")[1]); |
There was a problem hiding this comment.
From the owner's open source, looks like it is using GHApp instead of GithubBuilder for this logic: https://github.com/hub4j/github-api/blob/main/src/main/java/org/kohsuke/github/GHApp.java
I don't see such getApp() function be mentioned in GithubBuilder: https://github.com/hub4j/github-api/blob/main/src/main/java/org/kohsuke/github/GitHubBuilder.java
There was a problem hiding this comment.
I believe this is the one: https://github.com/hub4j/github-api/blob/af5bde714c3f771f6793392343ff0187bd8180df/src/main/java/org/kohsuke/github/GitHub.java#L1184. We are calling the method from the object type Github.
There was a problem hiding this comment.
But github is a GithubBuilder object, not Github though, right?
There was a problem hiding this comment.
Pls refer to https://github.com/AvvariSaiBharadwaj/dockerfile-image-update/blob/excludeRenovateRepos/dockerfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/GithubAppCheck.java#L45-L48. github is generated by githubBuilder.build() which returns a github object (https://github.com/hub4j/github-api/blob/main/src/main/java/org/kohsuke/github/GitHubBuilder.java#L508)
| try { | ||
| gitHub.getApp().getInstallationByRepository(fullRepoName.split("/")[0], fullRepoName.split("/")[1]); | ||
| return true; | ||
| } catch (HttpException exception) { |
There was a problem hiding this comment.
If we're defaulting to false, there's a risk of false negatives.
There was a problem hiding this comment.
Yes, but I am trying to be cautious. Since we are dealing with docker patches, isn't it better to get both DFIU and Renovate PRs and have a confusing experience, rather than not receive either PR and fall behind on your patches?
| try { | ||
| generateJWT(appId, privateKeyPath); | ||
| } catch (IOException | GeneralSecurityException exception) { | ||
| log.warn("Could not refresh the JWT due to exception: {}", exception.getMessage()); |
There was a problem hiding this comment.
Wouldn't this cause a problem? If we simply ignore this exception and default the value to false in the isGithubAppEnabledOnRepository method, it means that if the JWT refresh fails, we'll end up creating PRs even if the repository isn't onboarded to Renovate.
There was a problem hiding this comment.
Same comment as above. I am trying to be cautious. Lmk if you think otherwise.
There was a problem hiding this comment.
Yes, but I am trying to be cautious. Since we are dealing with docker patches, isn't it better to get both DFIU and Renovate PRs and have a confusing experience, rather than not receive either PR and fall behind on your patches?
I agree with Sai in this case
| eq(gitHubContentToProcess), anyList(), eq(gitForkBranch), | ||
| eq(rateLimiter)); | ||
| } | ||
| // @Test |
There was a problem hiding this comment.
Working on fixing them. Wanted to get the code reviewed beforehand.
There was a problem hiding this comment.
@jeetchoudhary added the unit tests with the change
| @@ -112,6 +112,14 @@ static ArgumentParser getArgumentParser() { | |||
| .setDefault(false) //To prevent null from being returned by the argument | |||
| .required(false) | |||
| .help("Enable debug logging, including git wire logs."); | |||
| parser.addArgument("--" + SKIP_GITHUB_APP_ID) | |||
| .type(String.class) | |||
There was a problem hiding this comment.
Aren't IDs always integers?
There was a problem hiding this comment.
@afalko It does not matter as it would be parsed as a parameter in a request API call to Github API to generate a JWT token. We have tested this
There was a problem hiding this comment.
Why not have the parser do the parsing for you upfront here?
There was a problem hiding this comment.
@afalko the reason for that is because appID will be parsed into the JWT token generation API call under the .withIssuer(appId) subcall, and this has to be parsed as a string type. You can see more here: https://www.baeldung.com/java-auth0-jwt
We can change it to Integer type if you truly think it is necessary and purposeful (to comply with the App ID integer type from Github App) and convert it to string when parsed into this API call
There was a problem hiding this comment.
Gotcha - pass-through might be fine, but it likely be a difficult error message to understand (vs. hey, you need an integer here). I think it is purposeful to make this integer for that release, but not necessary :)
|
Thanks for the contribution! It looks like @saiavvari @hanelliot-sfdc is an internal user so signing the CLA is not required. However, we need to confirm this. |
| jwt = JWT.create() | ||
| .withIssuer(appId) | ||
| .withIssuedAt(Date.from(now)) | ||
| .withExpiresAt(Date.from(now.plusSeconds(600))) // 10 minutes expiration |
There was a problem hiding this comment.
what will the behavior be if you set this really short (for example 1 second)
There was a problem hiding this comment.
@afalko we would like to avoid as much API calls as possible. Setting to 1 second means that we have to make an API call to regenerate a new JWT token every 1 second. Taking into account that the DFIU takes hours to run, I believe it is not efficient
ghost
force-pushed
the
excludeRenovateRepos
branch
from
463a753 to
9685288
Compare
Usage: docker compose [OPTIONS] COMMAND
Define and run multi-container applications with Docker
Options:
--all-resources Include all resources, even those not used by services
--ansi string Control when to print ANSI control characters ("never"|"always"|"auto") (default "auto")
--compatibility Run compose in backward compatibility mode
--dry-run Execute command in dry run mode
--env-file stringArray Specify an alternate environment file
-f, --file stringArray Compose configuration files
--parallel int Control max parallelism, -1 for unlimited (default -1)
--profile stringArray Specify a profile to enable
--progress string Set type of progress output (auto, tty, plain, quiet) (default "auto")
--project-directory string Specify an alternate working directory
(default: the path of the, first specified, Compose file)
-p, --project-name string Project name
Commands:
attach Attach local standard input, output, and error streams to a service's running container
build Build or rebuild services
config Parse, resolve and render compose file in canonical format
cp Copy files/folders between a service container and the local filesystem
create Creates containers for a service
down Stop and remove containers, networks
events Receive real time events from containers
exec Execute a command in a running container
images List images used by the created containers
kill Force stop service containers
logs View output from containers
ls List running compose projects
pause Pause services
port Print the public port for a port binding
ps List containers
pull Pull service images
push Push service images
restart Restart service containers
rm Removes stopped service containers
run Run a one-off command on a service
scale Scale services
start Start services
stats Display a live stream of container(s) resource usage statistics
stop Stop services
top Display the running processes
unpause Unpause services
up Create and start containers
version Show the Docker Compose version information
wait Block until the first service container stops
watch Watch build context for service and rebuild/refresh containers when files are updated
Run 'docker compose COMMAND --help' for more information on a command.
github-actions
bot
added
the
chore
|
See this issue regarding the |
No description provided.