[GitHub] [beam] youngoli commented on a change in pull request #16228: [BEAM-13399] Add check for dev versions of JARs to download code

[GitBox <gi...@apache.org>]

youngoli commented on a change in pull request #16228:
URL: https://github.com/apache/beam/pull/16228#discussion_r770172513



##########
File path: sdks/go/pkg/beam/core/runtime/xlangx/expansionx/download.go
##########
@@ -58,6 +58,10 @@ func GetBeamJar(gradleTarget, version string) (string, error) {
 }
 
 func (j *jarGetter) getJar(gradleTarget, version string) (string, error) {
+	if strings.Contains(version, ".dev") {

Review comment:
       While I support having a clearer error message for users, it feels a bit limiting to just disallow any container with ".dev" outright. What about approaching this as letting the code fail naturally, and if it fails and we detect that the user was attempting to retrieve a .dev container, then we add this message.
   
   You'd need to run this code with a .dev container to see where exactly it fails (probably on http.Get if I had to guess). But then all you have to do is move this if statement to that error block, and if it's true then wrapping the error with a [SetTopLevelMsg](https://github.com/apache/beam/blob/master/sdks/go/pkg/beam/internal/errors/errors.go#L90) function should do it. (I originally wrote that function for this exact purpose; adding messages directly to users to tell them how to fix a specific error).
   
   That changes this behavior from "disallowing something" to "detecting a common error case".




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org