Notify users if there is a new version of preflight #1103

skattoju · 2023-12-07T16:14:16Z

fixes #1043

Currently this is only done when running check container.
There is an option for specifying a Github auth token in case rate limiting becomes an issue. As of now its 60 request per hour unauthenticated which goes up to 5000 when using an auth token. In any case remaining requests are printed in the logs
The timeout for requests from the Github client to Github is set to one second to avoid waiting too long in case Github is slow to respond.

openshift-ci · 2023-12-07T16:14:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: skattoju

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [skattoju]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

dcibot · 2023-12-07T16:26:16Z

FAILURE https://www.distributed-ci.io/jobs/13dbdc6e-e144-4ada-be21-7de33435a730/jobStates
from Notify users if there is a new version of preflight #1103

dcibot · 2023-12-07T17:30:49Z

FAILURE https://www.distributed-ci.io/jobs/d244cc9c-db35-4eb9-ac57-4b9be09a2e83/jobStates
from Notify users if there is a new version of preflight #1103

dcibot · 2023-12-18T13:35:45Z

SUCCESS https://www.distributed-ci.io/jobs/d0834e4f-fa60-4cd4-a79a-bc5af7786c2d/jobStates
from Notify users if there is a new version of preflight #1103

dcibot · 2023-12-18T16:15:50Z

SUCCESS https://www.distributed-ci.io/jobs/46ef51d9-fa89-4a75-af5c-292ee794abc6/jobStates
from Notify users if there is a new version of preflight #1103

dcibot · 2023-12-19T17:32:56Z

SUCCESS https://www.distributed-ci.io/jobs/995d47ef-eb74-43a0-a2fc-e2173f70b524/jobStates
from Notify users if there is a new version of preflight #1103

dcibot · 2024-01-09T19:04:20Z

SUCCESS https://www.distributed-ci.io/jobs/d135658a-6827-4cd2-98ec-acd3d01ae25d/jobStates
from Notify users if there is a new version of preflight #1103

acornett21 · 2024-01-11T14:49:05Z

cmd/preflight/cmd/check_container.go

@@ -198,7 +204,7 @@ func checkContainerRunE(cmd *cobra.Command, args []string, runpreflight runPrefl

 func checkContainerPositionalArgs(cmd *cobra.Command, args []string) error {
 	if len(args) != 1 {
-		return fmt.Errorf("a container image positional argument is required")
+		return fmt.Errorf("a container image positional argument is required, found %v", args)


Is this updated needed for this task? It seems like an enhancement that might need it's own PR.

i figured no harm in keeping it 🤷‍♂️

Agreed with @acornett21. This change doesn't appear necessary. Please revert this change.

acornett21 · 2024-01-11T14:54:23Z

cmd/preflight/cmd/check_container.go

@@ -257,6 +270,38 @@ func validateCertificationProjectID(cmd *cobra.Command, args []string) error {
 	return nil
 }

+// checkForNewerReleaseVersion checks if there is a newer release available
+func checkForNewerReleaseVersion(cmd *cobra.Command) {
+	logger, err := logr.FromContext(cmd.Context())


I think using logr.FromContextOrDiscard might be a better choice, that way we don't have to worry about errors. But I'll let other's chime in as well.

If you're going to ignore the error, you may as well use logr.FromContextOrDiscard here. Not having a logger should not translate to not running this check.

acornett21 · 2024-01-11T14:59:39Z

cmd/preflight/cmd/check_container.go

+	ghToken, err := cmd.Flags().GetString("gh-auth-token")
+	if err == nil && len(ghToken) > 0 {
+		client = github.NewClient(&http.Client{
+			// Timeout in 1s in case Github is slow to respond
+			Timeout: time.Second * 1,
+		}).WithAuthToken(ghToken)
+	} else {
+		client = github.NewClient(&http.Client{
+			// timeout in 1s in case Github is slow to respond
+			Timeout: time.Second * 1,
+		})
+	}


I think this should be simplified, sudo code below

client := github.NewClient(nil) // this lets gh initialize a client we do this in other apps if check for token { client = client.WithAuthToken(ghToken) }

I wanted to keep the time out short in order not to interfere with CI when github has issues

I really don't think we should add the entire GitHub client solely to use the token. Do we really foresee people running into the GitHub rate limit?

We should probably just be hitting it with net/http without bringing in the github client library. If we get reports that people are hitting it, we can address it then.

I think we said earlier that hitting the rate limit would happen quite often to people running the tool in CI behind a VPN and that was the reason to use GH client and have the option to pass a token.

acornett21 · 2024-01-11T15:02:29Z

version/version.go

+
+func (vc *VersionContext) LatestReleasedVersion(cmd *cobra.Command, svc VersionClient) (*github.RepositoryRelease, error) {
+	ctx := cmd.Context()
+	logger, err := logr.FromContext(ctx)


same logr.FromContextOrDiscard here

acornett21 · 2024-01-11T15:06:24Z

version/version.go


 var (
 	projectName = "github.com/redhat-openshift-ecosystem/openshift-preflight"
-	version     = "unknown"
+	version     = "0.0.1"


Was this just changed so that other tests became valid?

yes this was for tests

Can we add a comment next to this and anywhere else that changed to 0.0.1 so we know it has to stay that value?

Its not implicit ? Ok will add a comment 👍

I don't think it's implicit, since it's been unknown or 0.0.0 locally forever.

I would rather this remain its original value "unknown". We inject this at build time, and we only truly care about whether someone is using an outdated version in cases where they've used our release binaries, which have this value populated.

Which test needs this to change?

I have tests comparing semver style versions thats why I changed it from unknown to 0.0.1. May be 0.0.0 is better ?

(The tests in version_test.go)

komish

Couple of changes/questions.

Overall, I'm wondering if we might be better off updating a cookie file somewhere on each release than querying the GitHub releases page.

I don't particularly love having to pass a GitHub API token into the tool for behavior that isn't core to Preflight's execution. That seems distracting to the end user, and I'm fairly confident it would be hit frequently since the only real client information GitHub would have is IP space. I think.

At any rate, I would much prefer making like an HTTP call that's hard-code to pull a value. Maybe it's a gh-pages thing. Not sure what I would want to see.

Some food for thought. cc @acornett21 @bcrochet

komish · 2024-01-17T17:44:01Z

cmd/preflight/cmd/check_container.go

@@ -198,7 +204,7 @@ func checkContainerRunE(cmd *cobra.Command, args []string, runpreflight runPrefl

 func checkContainerPositionalArgs(cmd *cobra.Command, args []string) error {
 	if len(args) != 1 {
-		return fmt.Errorf("a container image positional argument is required")
+		return fmt.Errorf("a container image positional argument is required, found %v", args)


Agreed with @acornett21. This change doesn't appear necessary. Please revert this change.

komish · 2024-01-17T19:48:55Z

cmd/preflight/cmd/check_container.go

+// validateConditions run all pre-run functions
+func validateConditions(cmd *cobra.Command, args []string) error {
+	err := validateCertificationProjectID()
+	checkForNewerReleaseVersion(cmd)
+	return err
+}
+


Instead of writing another function that we call, does it make sense to reuse cobra.MatchAll? Technically, it's to be used for the PositionalArgs struct key, but the signature looks the same to me.

cc @acornett21 @bcrochet

I concur here. It seems odd here to call another function between setting err and returning it. Not sure about the use of MatchAll. But it wouldn't hurt to try it. Otherwise, at the very least I would suggest something like

Suggested change

// validateConditions run all pre-run functions

func validateConditions(cmd *cobra.Command, args []string) error {

err := validateCertificationProjectID()

checkForNewerReleaseVersion(cmd)

return err

}

// validateConditions run all pre-run functions

func validateConditions(cmd *cobra.Command, args []string) error {

checkForNewerReleaseVersion(cmd)

return validateCertificationProjectID()

}

komish · 2024-01-17T19:49:39Z

cmd/preflight/cmd/check_container.go

@@ -257,6 +270,38 @@ func validateCertificationProjectID(cmd *cobra.Command, args []string) error {
 	return nil
 }

+// checkForNewerReleaseVersion checks if there is a newer release available
+func checkForNewerReleaseVersion(cmd *cobra.Command) {
+	logger, err := logr.FromContext(cmd.Context())


If you're going to ignore the error, you may as well use logr.FromContextOrDiscard here. Not having a logger should not translate to not running this check.

komish · 2024-01-17T19:54:36Z

cmd/preflight/cmd/check_container_test.go

+
+	"github.com/redhat-openshift-ecosystem/openshift-preflight/artifacts"
+	"github.com/redhat-openshift-ecosystem/openshift-preflight/certification"
+	"github.com/redhat-openshift-ecosystem/openshift-preflight/internal/check"
+	"github.com/redhat-openshift-ecosystem/openshift-preflight/internal/cli"
+	"github.com/redhat-openshift-ecosystem/openshift-preflight/internal/formatters"
+	"github.com/redhat-openshift-ecosystem/openshift-preflight/internal/lib"
+	"github.com/redhat-openshift-ecosystem/openshift-preflight/internal/viper"


We do know why these are changing?

looks like just the order is changing

Well, it's not correct. The project-local imports should be after stdlib, not last. Please figure out if it's your IDE doing that, and update its settings accordingly.

komish · 2024-01-17T20:34:18Z

version/version.go

+
+	"github.com/go-logr/logr"
+	"github.com/google/go-github/v57/github"
+	semvc "github.com/hashicorp/go-version"


Is there any particular version why we're using this, and not https://pkg.go.dev/golang.org/x/mod/semver, or https://github.com/Masterminds/semver

Is one of these already used in a project we manage? If so I'd vote for picking that one.

changed to Masterminds 👍

komish · 2024-01-17T20:36:11Z

version/version.go

@@ -25,3 +38,35 @@ type VersionContext struct {
 func (vc *VersionContext) String() string {
 	return fmt.Sprintf("%s <commit: %s>", vc.Version, vc.Commit)
 }
+
+func (vc *VersionContext) LatestReleasedVersion(cmd *cobra.Command, svc VersionClient) (*github.RepositoryRelease, error) {


This shouldn't need *cobra.Command to function. If you need a context, accept a context.

With that said, I would say we probably don't want this to log in any way. It likely makes more sense to emit an error here, and have the caller figure out what to do with it.

Also, does this need to be a mod of the VersionContext struct for any reason? I think there's certainly value here in hard-coding some of the relevant values for our project ( e.g. the org/project values), but we're not accomplishing that by adding this as a method to the struct.

Make this an library function that accepts the parameters it needs to execute, then call it with the proper parameters provided at your call point(s).

If we make this just a generic library call that accepts the client you need to make to resolve the version, it may also simplify testing of this logic.

Agreed on the context here. cobra.Command should not have made it out of cmd package.

komish · 2024-01-17T20:46:18Z

version/version.go


 var (
 	projectName = "github.com/redhat-openshift-ecosystem/openshift-preflight"
-	version     = "unknown"
+	version     = "0.0.1"


I would rather this remain its original value "unknown". We inject this at build time, and we only truly care about whether someone is using an outdated version in cases where they've used our release binaries, which have this value populated.

Which test needs this to change?

acornett21 · 2024-01-17T21:26:45Z

I don't particularly love having to pass a GitHub API token into the tool for behavior that isn't core to Preflight's execution.

@komish The API key should be optional and not required. It would only be if a user experiences rate limiting from GH. Say they are using preflight in their CI system (which already calls GH a bunch).

komish · 2024-01-17T21:29:33Z

@komish The API key should be optional and not required. It would only be if a user experiences rate limiting from GH. Say they are using preflight in their CI system (which already calls GH a bunch).

Understood, but if we made a web request to GitHub's public-facing HTTP endpoints instead of the API, we could avoid having to request an API from the caller altogether. We don't use the token for anything else - just the version check. That feels strange to me.

[edit]: 100% believe it's necessary if we must make an API call to GitHub to get this information. There's no other way around it, we have to request it from the user so that they don't eventually complain they're getting rate limited and have no way of resolving. But having it be an input we need for something that doesn't impact core logic is what I prefer to avoid.

dcibot · 2024-01-17T21:40:16Z

SUCCESS https://www.distributed-ci.io/jobs/acc802d6-c31f-443d-afb0-55db7ee41c46/jobStates
from Notify users if there is a new version of preflight #1103

coveralls · 2024-02-19T15:19:47Z

coverage: 81.768% (+0.03%) from 81.735%
when pulling c7babfb on skattoju:latest_available_version
into 068aa75 on redhat-openshift-ecosystem:main.

dcibot · 2024-02-19T15:53:20Z

from change #1103:

SUCCESS https://www.distributed-ci.io/jobs/61293306-3c78-4912-a203-94e04a57af48/jobStates

Signed-off-by: Sid Kattoju <[email protected]>

bcrochet

A few concurrences and some additional questions.

bcrochet · 2024-02-21T20:24:52Z

cmd/preflight/cmd/check_container.go

 	"os"
 	"path/filepath"
 	rt "runtime"
 	"strings"
+	"time"
+
+	"github.com/go-logr/logr"


This should not move like it is. Is the IDE moving this block up? Or is the linter suggesting that this be moved ahead of "local" imports?

The order should be:
stdlib

project

3rd party

bcrochet · 2024-02-21T20:29:50Z

cmd/preflight/cmd/check_container_test.go

+
+	"github.com/redhat-openshift-ecosystem/openshift-preflight/artifacts"
+	"github.com/redhat-openshift-ecosystem/openshift-preflight/certification"
+	"github.com/redhat-openshift-ecosystem/openshift-preflight/internal/check"
+	"github.com/redhat-openshift-ecosystem/openshift-preflight/internal/cli"
+	"github.com/redhat-openshift-ecosystem/openshift-preflight/internal/formatters"
+	"github.com/redhat-openshift-ecosystem/openshift-preflight/internal/lib"
+	"github.com/redhat-openshift-ecosystem/openshift-preflight/internal/viper"


Well, it's not correct. The project-local imports should be after stdlib, not last. Please figure out if it's your IDE doing that, and update its settings accordingly.

bcrochet · 2024-02-21T20:32:34Z

version/version.go

@@ -25,3 +38,35 @@ type VersionContext struct {
 func (vc *VersionContext) String() string {
 	return fmt.Sprintf("%s <commit: %s>", vc.Version, vc.Commit)
 }
+
+func (vc *VersionContext) LatestReleasedVersion(cmd *cobra.Command, svc VersionClient) (*github.RepositoryRelease, error) {


Agreed on the context here. cobra.Command should not have made it out of cmd package.

bcrochet · 2024-02-21T20:34:53Z

cmd/preflight/cmd/check_container.go

+// validateConditions run all pre-run functions
+func validateConditions(cmd *cobra.Command, args []string) error {
+	err := validateCertificationProjectID()
+	checkForNewerReleaseVersion(cmd)
+	return err
+}
+


I concur here. It seems odd here to call another function between setting err and returning it. Not sure about the use of MatchAll. But it wouldn't hurt to try it. Otherwise, at the very least I would suggest something like

Suggested change

// validateConditions run all pre-run functions

func validateConditions(cmd *cobra.Command, args []string) error {

err := validateCertificationProjectID()

checkForNewerReleaseVersion(cmd)

return err

}

// validateConditions run all pre-run functions

func validateConditions(cmd *cobra.Command, args []string) error {

checkForNewerReleaseVersion(cmd)

return validateCertificationProjectID()

}

bcrochet · 2024-02-21T20:40:51Z

cmd/preflight/cmd/check_container.go

+		})
+	}
+	// check if a newer release is available
+	latestRelease, err := version.Version.LatestReleasedVersion(cmd, client.Repositories)


This is awkward. LatestReleasedVersion, as @komish stated elsewhere, really doesn't need to be a part of Version.

Suggested change

latestRelease, err := version.Version.LatestReleasedVersion(cmd, client.Repositories)

latestRelease, err := version.LatestRelease(cmd.Context(), client.Repositories)

bcrochet · 2024-02-21T20:44:47Z

cmd/preflight/cmd/check_container.go

+	ghToken, err := cmd.Flags().GetString("gh-auth-token")
+	if err == nil && len(ghToken) > 0 {
+		client = github.NewClient(&http.Client{
+			// Timeout in 1s in case Github is slow to respond
+			Timeout: time.Second * 1,
+		}).WithAuthToken(ghToken)
+	} else {
+		client = github.NewClient(&http.Client{
+			// timeout in 1s in case Github is slow to respond
+			Timeout: time.Second * 1,
+		})
+	}


I really don't think we should add the entire GitHub client solely to use the token. Do we really foresee people running into the GitHub rate limit?

We should probably just be hitting it with net/http without bringing in the github client library. If we get reports that people are hitting it, we can address it then.

acornett21 · 2024-02-21T20:55:26Z

I don't think we'll need this PR anymore, since the business's new ask is to block on submission which will be handled by pyxis. And preflights high level responsibilities for that are now:

Handle and display the response.
Add/Update our GitHub Release Action to post to pyxis the version that was released.

openshift-ci · 2024-02-21T20:55:56Z

@skattoju: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/4.11-e2e	`831edc7`	link	true	`/test 4.11-e2e`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

skattoju · 2024-02-21T21:45:05Z

Closing this since version enforcement will be implemented in the backend (pyxis) ref: CLOUDWF-10041

openshift-ci bot requested review from jomkz and tonytcampbell December 7, 2023 16:14

skattoju marked this pull request as draft December 7, 2023 16:14

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 7, 2023

skattoju force-pushed the latest_available_version branch 2 times, most recently from fb91968 to d28dec9 Compare December 7, 2023 17:19

skattoju force-pushed the latest_available_version branch from d28dec9 to e65d008 Compare December 17, 2023 18:13

skattoju force-pushed the latest_available_version branch 3 times, most recently from dd746b7 to 67697ee Compare December 18, 2023 15:38

skattoju marked this pull request as ready for review December 18, 2023 15:57

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 18, 2023

openshift-ci bot requested review from acornett21 and komish December 18, 2023 15:57

skattoju requested review from bcrochet and removed request for acornett21 December 18, 2023 15:57

skattoju force-pushed the latest_available_version branch from 67697ee to dee20f5 Compare December 19, 2023 16:23

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 5, 2024

skattoju force-pushed the latest_available_version branch from dee20f5 to 4fbba38 Compare January 9, 2024 17:59

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 9, 2024

acornett21 reviewed Jan 11, 2024

View reviewed changes

acornett21 requested changes Jan 11, 2024

View reviewed changes

openshift-ci bot assigned acornett21 Jan 11, 2024

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 12, 2024

skattoju force-pushed the latest_available_version branch from 4fbba38 to 465434a Compare January 17, 2024 20:45

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 17, 2024

komish requested changes Jan 17, 2024

View reviewed changes

openshift-ci bot assigned komish Jan 17, 2024

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 23, 2024

skattoju force-pushed the latest_available_version branch from 465434a to aefbfe0 Compare February 19, 2024 15:01

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 19, 2024

skattoju force-pushed the latest_available_version branch from aefbfe0 to c7babfb Compare February 19, 2024 15:16

skattoju force-pushed the latest_available_version branch from c7babfb to 229ef32 Compare February 21, 2024 19:40

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 21, 2024

add check for latest available version

831edc7

Signed-off-by: Sid Kattoju <[email protected]>

skattoju force-pushed the latest_available_version branch from 229ef32 to 831edc7 Compare February 21, 2024 19:47

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 21, 2024

bcrochet requested changes Feb 21, 2024

View reviewed changes

openshift-ci bot assigned bcrochet Feb 21, 2024

skattoju closed this Feb 21, 2024

	latestRelease, err := version.Version.LatestReleasedVersion(cmd, client.Repositories)
	latestRelease, err := version.LatestRelease(cmd.Context(), client.Repositories)

Notify users if there is a new version of preflight #1103

Notify users if there is a new version of preflight #1103

Conversation

skattoju commented Dec 7, 2023 • edited Loading

openshift-ci bot commented Dec 7, 2023

dcibot commented Dec 7, 2023

dcibot commented Dec 7, 2023

dcibot commented Dec 18, 2023

dcibot commented Dec 18, 2023

dcibot commented Dec 19, 2023

dcibot commented Jan 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

komish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acornett21 commented Jan 17, 2024 • edited Loading

komish commented Jan 17, 2024 • edited Loading

dcibot commented Jan 17, 2024

coveralls commented Feb 19, 2024

dcibot commented Feb 19, 2024

bcrochet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acornett21 commented Feb 21, 2024

openshift-ci bot commented Feb 21, 2024

skattoju commented Feb 21, 2024

skattoju commented Dec 7, 2023 •

edited

Loading

acornett21 commented Jan 17, 2024 •

edited

Loading

komish commented Jan 17, 2024 •

edited

Loading