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

Catching IOException when the image name is invalid #320

Merged
merged 1 commit into from
Feb 24, 2022
Merged

Catching IOException when the image name is invalid #320

merged 1 commit into from
Feb 24, 2022

Conversation

avimanyum
Copy link
Contributor

@avimanyum avimanyum commented Feb 23, 2022

On running the All subcommand, if there is an image in the tag store that has an invalid image name, the findFilesWithImage method throws an IOException here. However, in the All subcommand execute path we are not catching this exception. As a result, if the image tag store has one entry with an invalid image name, the all subcommand will exit once it hits that entry and not process the following entries.
This fix allows us to catch these exceptions and move on to processing the following images in the image tag store.

@codecov
Copy link

codecov bot commented Feb 23, 2022

Codecov Report

Merging #320 (de973c0) into master (e18ae3e) will not change coverage.
The diff coverage is 100.00%.

❗ Current head de973c0 differs from pull request most recent head 3acdd13. Consider uploading reports for the commit 3acdd13 to get more accurate results

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #320   +/-   ##
=========================================
  Coverage     81.47%   81.47%           
  Complexity      263      263           
=========================================
  Files            19       19           
  Lines           934      934           
  Branches        126      126           
=========================================
  Hits            761      761           
  Misses          142      142           
  Partials         31       31           
Impacted Files Coverage Δ
...ce/dockerfileimageupdate/subcommands/impl/All.java 93.10% <100.00%> (ø)

@@ -74,7 +74,7 @@ public void execute(final Namespace ns, final DockerfileGitHubUtil dockerfileGit
}
});
}
} catch (GHException | HttpException e){
Copy link
Collaborator

Choose a reason for hiding this comment

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

You are sure we don't still also need HttpException? I guess IOException is a superset 🤔

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, it is!

Copy link
Collaborator

@afalko afalko left a comment

Choose a reason for hiding this comment

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

I think you forgot to clean up the import on httpexception

@avimanyum avimanyum added the bug Bug fixes label Feb 24, 2022
@justinharringa
Copy link
Contributor

Could you provide more information on what the scenario is here? It's unclear what is happening and why this should be an exception rather than a validation.

@avimanyum
Copy link
Contributor Author

Could you provide more information on what the scenario is here? It's unclear what is happening and why this should be an exception rather than a validation.

Added here

@justinharringa
Copy link
Contributor

Could you provide more information on what the scenario is here? It's unclear what is happening and why this should be an exception rather than a validation.

Added here

What do we mean by an "invalid image name"? It doesn't exist? The format is not correct? Examples would be useful if you could redact them.

Thanks again!

Copy link
Collaborator

@afalko afalko left a comment

Choose a reason for hiding this comment

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

We are good. We * add exception

@avimanyum
Copy link
Contributor Author

Could you provide more information on what the scenario is here? It's unclear what is happening and why this should be an exception rather than a validation.

Added here

What do we mean by an "invalid image name"? It doesn't exist? The format is not correct? Examples would be useful if you could redact them.

Thanks again!

Yeah sure. This is the logic we have to determine the validity of an image name. An example of an invalid image name would be a:a. This happened to be in a tag store that the canary instance was using. That's how I encountered this bug :)

@justinharringa
Copy link
Contributor

weird... cool thanks for the info 😄 might be helpful for others too

@avimanyum avimanyum merged commit 57b8b66 into salesforce:master Feb 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants