Skip to content

Adding support for Oracle Cloud#3352

Merged
sdedic merged 1 commit intoapache:masterfrom
jhorvath:oci-support
Jan 17, 2022
Merged

Adding support for Oracle Cloud#3352
sdedic merged 1 commit intoapache:masterfrom
jhorvath:oci-support

Conversation

@jhorvath
Copy link
Copy Markdown
Member

@jhorvath jhorvath commented Dec 2, 2021

Adding new module with support for Oracle Cloud. For now it is able to browser Tenancy and Compartments. In selected Compartment it shows Autonomous Databases and allow to create and download Database Wallet.

Copy link
Copy Markdown

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

Modify nbbuild/cluster.properties to include this module. Modify nbbuild/build.properties to include the new Javadoc. Hopefully then the module becomes subject to license & co. checks.

}

/**
* Creates and downloads a wallet for the specified Autonomous Database.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Where the downloaded wallet is stored?

Copy link
Copy Markdown

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

Wrapping libraries into modules, updating versions of some libraries - looks like good job. Creating and using the @CloudProvider looks fine as well. I don't see problems from architecture perspective.

*
* @author Jan Horvath
*/
public class OCIItem {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The OCIItem would then be the API of this module... but right now it would be private API as the class isn't exposed.

}

@Override
public void stateChanged(ChangeEvent e) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks like empty implementation.

Copy link
Copy Markdown

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

What's the license for enterprise/cloud.oracle/src/org/netbeans/modules/cloud/oracle/resources/orcl.png? There should be licenseinfo.xml file specifying that. Btw. are you sure the compartment and database .svg files are really licensed under Apache license?

@JaroslavTulach
Copy link
Copy Markdown

Except the image licenses (which Jan Horváth will clarify) I consider the integration solid. Any other review comments? Otherwise I plan to merge this week.

Copy link
Copy Markdown
Contributor

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

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

As @JaroslavTulach already pointed out the PNG needs a license to it enterprise/cloud.oracle/src/org/netbeans/modules/cloud/oracle/resources/orcl.png.

The rest of the changes looks ok to me (only eyeballed!).

For the library updates, this duplicates/overrides #2287. It would be good if the bcpg module would then also be updated.

In general this PR needs cleanup, we should not merge this with 28 commits. After review and before merge these should either be fully squashed into a single commit or squashed based on logical units.

Copy link
Copy Markdown
Contributor

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

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

Before this is merged PLEASE SQUASH!

@sdedic
Copy link
Copy Markdown
Member

sdedic commented Jan 17, 2022

All green two approvals, changes squashed. Merging.

@sdedic sdedic merged commit a46aa48 into apache:master Jan 17, 2022
@neilcsmith-net neilcsmith-net added this to the NB13 milestone Jan 17, 2022
@neilcsmith-net
Copy link
Copy Markdown
Member

Thanks @sdedic Please make sure to check and add the milestone when merging.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants