Skip to content

remove xray version check #2

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

remove xray version check #2

wants to merge 1 commit into from

Conversation

aliculPix4D
Copy link

@aliculPix4D aliculPix4D commented Jul 23, 2025 •
edited
Loading

I think the simplest is to just remove this check.

Basically, I rebuilt the provider and tested locally if it works. No errors were raised in dev workspace without active licence.

Before we had:

╷
│ Error: Error getting Xray version
│ 
│   with provider["registry.terraform.io/jfrog/xray"],
│   on main.tf line 69, in provider "xray":
│   69: provider "xray" {
│ 
│ failed to get Xray version.

even when we didn't try to create any resources..

Alternative:

  • add another option flag in e.g skip_xray_version_check with default value false (keeping the same behaviour as today)
// XrayProviderModel describes the provider data model.
type XrayProviderModel struct {
	Url                  types.String `tfsdk:"url"`
	AccessToken          types.String `tfsdk:"access_token"`
	OIDCProviderName     types.String `tfsdk:"oidc_provider_name"`
	TFCCredentialTagName types.String `tfsdk:"tfc_credential_tag_name"`
        SkipXrayVersionCheck types.Bool `tfsdk:"skip_xray_version_check"`
}

then in our terraform provider config we would just set: skip_xray_version_check: local.use_licence ? true : false

This was actually behaviour of artifactory provider long time ago. They had: check_licence or similar...

@aliculPix4D aliculPix4D requested a review from a team as a code owner July 23, 2025 13:05
@aliculPix4D aliculPix4D requested review from odormond, iAmoric, SpectreVert and marco-m-pix4d and removed request for a team July 23, 2025 13:05
aliculPix4D
aliculPix4D commented Jul 23, 2025
View reviewed changes
pkg/xray/provider.go
featureUsage := fmt.Sprintf("Terraform/%s", req.TerraformVersion)
go util.SendUsage(ctx, restyClient.R(), productId, featureUsage)

meta := util.ProviderMetadata{
Client: restyClient,
ProductId: productId,
XrayVersion: version,
Copy link
Author

Choose a reason for hiding this comment

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

this really didn't seems to be used for anything except reporting metadata.

Base automatically changed from pci-4198-add-codeowners to main July 23, 2025 13:14
@marco-m-pix4d
Copy link

It is unclear to me what is the place of this PR in the whole problem we were discussing.

  • Is this a temporary workaround, and other work is planned?
  • Is this the same we want to propose upstream?
  • I am missing something?

odormond
odormond approved these changes Jul 24, 2025
View reviewed changes
pkg/xray/provider.go
)
return
}

featureUsage := fmt.Sprintf("Terraform/%s", req.TerraformVersion)
go util.SendUsage(ctx, restyClient.R(), productId, featureUsage)
Copy link

Choose a reason for hiding this comment

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

Maybe we could drop the telemetry too if we really need to keep this fork around. 😄

Copy link
Author

@aliculPix4D aliculPix4D Jul 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I noticed this immediately yesterday. Its just a post request towards our Artifactory:
https://github.com/jfrog/terraform-provider-shared/blob/55f41af49bcfba33502066064ac468c09563340c/util/util.go#L64C9-L64C37

The Artifactory REST API endpoint artifactory/api/system/usage provides information about the system's usage, including resource consumption. This endpoint is useful for monitoring and understanding how Artifactory is being utilized.

@aliculPix4D
Copy link
Author

aliculPix4D commented Jul 24, 2025 •
edited
Loading

It is unclear to me what is the place of this PR in the whole problem we were discussing.

@marco-m-pix4d see the issue:
jfrog#338
and a PR proposed upstream to address it:
jfrog#339

marco-m-pix4d
marco-m-pix4d approved these changes Jul 24, 2025
View reviewed changes
Copy link

@marco-m-pix4d marco-m-pix4d left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation.

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

@odormond odormond odormond approved these changes

@iAmoric iAmoric iAmoric approved these changes

@SpectreVert SpectreVert SpectreVert approved these changes

@marco-m-pix4d marco-m-pix4d marco-m-pix4d approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@aliculPix4D @marco-m-pix4d @odormond @iAmoric @SpectreVert