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

Add support for Terraform package type #354 #380

Merged
merged 5 commits into from
Nov 8, 2023
Merged

Add support for Terraform package type #354 #380

merged 5 commits into from
Nov 8, 2023

Conversation

nevenr
Copy link
Contributor

@nevenr nevenr commented Sep 27, 2023

Please review.

Thanks.

Regards,
N

@github-actions
Copy link

github-actions bot commented Sep 27, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@eyalbe4 eyalbe4 self-requested a review September 28, 2023 02:10
@nevenr
Copy link
Contributor Author

nevenr commented Sep 28, 2023

I have read the CLA Document and I hereby sign the CLA

@nevenr
Copy link
Contributor Author

nevenr commented Sep 28, 2023

recheck

@yahavi yahavi added improvement Automatically generated release notes safe to test Approve running integration tests on a pull request and removed safe to test Approve running integration tests on a pull request labels Oct 1, 2023
@yahavi yahavi added safe to test Approve running integration tests on a pull request and removed safe to test Approve running integration tests on a pull request labels Oct 1, 2023
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Oct 1, 2023
@yahavi yahavi added the safe to test Approve running integration tests on a pull request label Oct 1, 2023
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Oct 1, 2023
Copy link
Member

@yahavi yahavi 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 this contribution @nevenr!
Please consider my inline comments.


@Override
RepositorySettings getRepositorySettings(RepositoryType repositoryType) {

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

Comment on lines 23 to 51
if (repositoryType == RepositoryTypeImpl.REMOTE) {
def settings = new TerraformRepositorySettingsImpl()
settings.with {
repoLayout = defaultLayout
vcsType = VcsType.GIT
vcsGitProvider = VcsGitProvider.GITHUB
terraformRegistryUrl = "https://registry.terraform.io"
terraformProvidersUrl = "https://releases.hashicorp.com"
remoteRepoLayoutRef = defaultLayout
}
return settings
}

if (repositoryType == RepositoryTypeImpl.VIRTUAL) {
def settings = new TerraformRepositorySettingsImpl()
settings.with {
repoLayout = moduleLayout
}
return settings
}

if (repositoryType == RepositoryTypeImpl.FEDERATED) {
def settings = new TerraformRepositorySettingsImpl()
settings.with {
terraformType = TerraformRepositorySettings.TerraformType.module
repoLayout = moduleLayout
}
return settings
}
Copy link
Member

Choose a reason for hiding this comment

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

Let's use switch-case here

import org.jfrog.artifactory.client.model.repository.settings.vcs.VcsGitProvider;
import org.jfrog.artifactory.client.model.repository.settings.vcs.VcsType;

public interface TerraformRepositorySettings extends RepositorySettings{
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public interface TerraformRepositorySettings extends RepositorySettings{
@JsonIgnoreProperties(ignoreUnknown = true)
public interface TerraformRepositorySettings extends RepositorySettings {


public interface TerraformRepositorySettings extends RepositorySettings{

// local and federated settings
Copy link
Member

Choose a reason for hiding this comment

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

Let's capitalize all new comments

Suggested change
// local and federated settings
// Local and federated settings

@yahavi yahavi added the safe to test Approve running integration tests on a pull request label Nov 8, 2023
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Nov 8, 2023
@yahavi yahavi added new feature Automatically generated release notes safe to test Approve running integration tests on a pull request and removed improvement Automatically generated release notes labels Nov 8, 2023
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Nov 8, 2023
Copy link
Member

@yahavi yahavi left a comment

Choose a reason for hiding this comment

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

Thanks @nevenr!

@yahavi yahavi merged commit aa4d64d into jfrog:dev Nov 8, 2023
6 checks passed
@yahavi
Copy link
Member

yahavi commented Nov 9, 2023

@nevenr
Artifactory Java client 2.17.0 is released. This version includes the support for Terraform that you added.
We'd appreciate your feedback on that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature Automatically generated release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants