Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

[NXCM-5131] Fix itemMaxAge settings for non-maven repositories #787

Merged
merged 8 commits into from Apr 30, 2013

Conversation

Projects
None yet
4 participants
Contributor

nabcos commented Mar 25, 2013

This pull reverts the initial change for NXCM-5131, because it affected behavior of non-maven-repositories inheriting from AbstractProxyRepository. instead of the implicit default of itemMaxAge (1440 minutes), 0 would be used instead.

With this change, the repository resource sets itemMaxAge for repositories that do not have the maven-specific artifact/metadataMaxAge.

Contributor

ifedorenko commented Mar 25, 2013

Do we need a UT for "implicit default value of 1440 minutes is used"?

Contributor

nabcos commented Mar 25, 2013

After some more code reading, the previous fix does not break anything for "real life" situations:

The NuGet IT that fails creates the proxy repository with unset itemMaxAge, which would previously fall back to the implicit default for "no external configuration".

If you create a repository in the UI, that value will always be set explicitely, so there is no behavior change visible for end users. The itemMaxAge is actually used in NuGet repositories, and can be set and saved in the UI. (It will not be read back without this change to the RepositoryPlexusResource, though.)

@jdillon jdillon and 1 other commented on an outdated diff Apr 12, 2013

.../client/core/subsystem/repository/BaseRepository.java
@@ -0,0 +1,30 @@
+package org.sonatype.nexus.client.core.subsystem.repository;
+
+/**
+ * Base class for hosted/proxy repositories.
+ *
+ * @since 2.4
+ */
+interface BaseRepository<T extends Repository, S extends RepositoryStatus>
+ extends Repository<T, S>
@jdillon

jdillon Apr 12, 2013

Contributor

is this package private on purpose?

@nabcos

nabcos Apr 15, 2013

Contributor

Yes, I did not see much value in exposing that technical DRY-step as public API, but the intermediate step is needed IIRC because virtual and group repositories don't have that property.

@jdillon jdillon commented on an outdated diff Apr 12, 2013

plugins/restlet1x/nexus-restlet1x-testsuite/pom.xml
+ </parent>
+
+ <artifactId>nexus-restlet1x-testsuite</artifactId>
+
+ <dependencyManagement>
+
+ <dependencies>
+ <dependency>
+ <groupId>org.sonatype.nexus.plugins</groupId>
+ <artifactId>nexus-restlet1x-testsupport-plugin</artifactId>
+ <version>${project.version}</version>
+ <classifier>bundle</classifier>
+ <type>nexus-plugin</type>
+ </dependency>
+ </dependencies>
+ </dependencyManagement>
@jdillon

jdillon Apr 12, 2013

Contributor

this should be defined in nexus-restlet1x-parent

@jdillon jdillon commented on an outdated diff Apr 12, 2013

plugins/restlet1x/nexus-restlet1x-testsuite/pom.xml
+ <groupId>org.sonatype.nexus</groupId>
+ <artifactId>nexus-testsuite-support</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.sonatype.nexus.client</groupId>
+ <artifactId>nexus-client-core</artifactId>
+ </dependency>
+
+ <dependency>
+ <groupId>${it.nexus.bundle.groupId}</groupId>
+ <artifactId>${it.nexus.bundle.artifactId}</artifactId>
+ <version>${it.nexus.bundle.version}</version>
+ <scope>test</scope>
+ <type>zip</type>
+ <classifier>bundle</classifier>
+ </dependency>
@jdillon

jdillon Apr 12, 2013

Contributor

Why is this here, this should already be defined

@ifedorenko ifedorenko commented on the diff Apr 15, 2013

.../client/core/subsystem/repository/BaseRepository.java
+ *
+ * This program and the accompanying materials are made available under the terms of the Eclipse Public License Version 1.0,
+ * which accompanies this distribution and is available at http://www.eclipse.org/legal/epl-v10.html.
+ *
+ * Sonatype Nexus (TM) Professional Version is available from Sonatype, Inc. "Sonatype" and "Sonatype Nexus" are trademarks
+ * of Sonatype, Inc. Apache Maven is a trademark of the Apache Software Foundation. M2eclipse is a trademark of the
+ * Eclipse Foundation. All other trademarks are the property of their respective owners.
+ */
+package org.sonatype.nexus.client.core.subsystem.repository;
+
+/**
+ * Base class for hosted/proxy repositories.
+ *
+ * @since 2.5
+ */
+interface BaseRepository<T extends Repository, S extends RepositoryStatus>
@ifedorenko

ifedorenko Apr 15, 2013

Contributor

What's the point of this interface? Why 'browsable' cannot be not part of Repository?

@nabcos

nabcos Apr 15, 2013

Contributor

Virtual and group repositories don't have browsable.

@ifedorenko

ifedorenko Apr 15, 2013

Contributor

How is this related to artifact maxage?

@nabcos

nabcos Apr 15, 2013

Contributor

The IT checks default values for unset properties on repo creation, not only for itemMaxAge.

@ifedorenko ifedorenko commented on an outdated diff Apr 15, 2013

...exus/client/core/subsystem/repository/Repository.java
@@ -83,4 +83,9 @@
*/
T remove();
+ /**
+ * @return {@code true} if the repository is exposed, {@code false} otherwise.
+ * @since 2.4
@ifedorenko

ifedorenko Apr 15, 2013

Contributor

wrong @SInCE version (and in few other places)

@ifedorenko ifedorenko commented on the diff Apr 15, 2013

...st/repositories/AbstractRepositoryPlexusResource.java
@@ -392,22 +392,26 @@ public RepositoryProxyResource getRepositoryProxyRestModel( ProxyRepository repo
// This is a total hack to be able to retrieve this data from a non core repo if available
@ifedorenko

ifedorenko Apr 15, 2013

Contributor

This is just sick. :-( I know we had this marvel of software design for awhile now, but is there a chance to fix this properly, i.e. without resorting to reflection, and without rewriting half of nexus?

@cstamas

cstamas Apr 15, 2013

Contributor

Yes, just make UI use Templates for repo creation. In short, changes I was proposing long time ago:

  1. expose existing templates over REST
  2. Repo creation UI should offer them... so, instead as today (select provider "maven2" and repository policy "Released", plus implicitly selected kind when you pressed Add Hosted / Proxy / ...., just make UI show the list of existing (filtered) templates.
  3. a bit more work needed on templates

So, if we want to stick with Add Hosted / Proxy ... buttons, we could filter templates for HostedRepository, ProxyRepository etc, and then instead of setting "maven2" and "Release" (from corresponding drop downs), user would choose "Maven2 (hosted, release)" entry (which maps to org.sonatype.nexus.templates.repository.maven.Maven2HostedRepositoryTemplate.Maven2HostedRepositoryTemplate) and UI would just send back the used template ID.

Still, even if this would not require rewriting half of nexus, templates (org.sonatype.nexus.templates.repository package) and this resource would need some love, as then there still the need to "apply" the values to the configuration sent by UI to the template before template.create() is invoked (which actually creates live repo instance, and does everything needed (adds it to config, saves config etc).

Contributor

jdillon commented Apr 29, 2013

I'm not sure what to do about this, I don't know the core api well enough to say if this is good or if there is something better. I don't think this makes it worse however (its already pretty bad), and if this fixes the problem then we probably should merge it.

nabcos added a commit that referenced this pull request Apr 30, 2013

Merge pull request #787 from sonatype/nxcm-5131-nuget-config
[NXCM-5131] Fix itemMaxAge settings for non-maven repositories

@nabcos nabcos merged commit 5bf9b7a into master Apr 30, 2013

@nabcos nabcos deleted the nxcm-5131-nuget-config branch Apr 30, 2013

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