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

adding jfrog, conf and adding the correct publishers #258

Merged
merged 3 commits into from
Dec 2, 2020

Conversation

cdsap
Copy link
Owner

@cdsap cdsap commented Nov 22, 2020

Fixes for #256 #257

  • @MyDogTom I fixed the problem you mentioned, the Extension for kts for Influx, Base, Graph, Elastic Search was incorrect. Tests were building with Groovy and we were not able to catch the error.

  • Fixed the problem of mavenPlugin name artifact, now all the plugins have same id. Plugins redeployed

@cdsap cdsap added the 1.4 label Nov 22, 2020
@cdsap
Copy link
Owner Author

cdsap commented Nov 22, 2020

Additionally, I have added the JFrog extension required by the dependencies published in Bintray

@codecov
Copy link

codecov bot commented Nov 22, 2020

Codecov Report

Merging #258 (a477e9e) into master (7da0685) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #258   +/-   ##
=========================================
  Coverage     52.41%   52.41%           
  Complexity      269      269           
=========================================
  Files            96       96           
  Lines          1757     1757           
  Branches        244      244           
=========================================
  Hits            921      921           
  Misses          760      760           
  Partials         76       76           
Impacted Files Coverage Δ Complexity Δ
...ava/com/cdsap/talaiot/plugin/base/BaseExtension.kt 0.00% <ø> (ø) 0.00 <0.00> (ø)
...iot/plugin/elasticsearch/ElasticSearchExtension.kt 0.00% <ø> (ø) 0.00 <0.00> (ø)
...a/com/cdsap/talaiot/plugin/graph/GraphExtension.kt 0.00% <ø> (ø) 0.00 <0.00> (ø)
...cdsap/talaiot/plugin/influxdb/InfluxdbExtension.kt 0.00% <ø> (ø) 0.00 <0.00> (ø)
...talaiot/plugin/pushgateway/PushgatewayExtension.kt 0.00% <ø> (ø) 0.00 <0.00> (ø)
...sap/talaiot/plugin/rethinkdb/RethinkdbExtension.kt 0.00% <ø> (ø) 0.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7da0685...a477e9e. Read the comment docs.

Copy link
Collaborator

@MyDogTom MyDogTom left a comment

Choose a reason for hiding this comment

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

I'm not familiar with the functionality under buildplugins, but I checked Gradle portal and plugin id looks fine. (https://plugins.gradle.org/plugin/com.cdsap.talaiot.plugin.influxdb)
I'll be able to verify redeployed version tomorrow in the evening.

@@ -11,7 +10,7 @@ open class InfluxdbExtension(project: Project) : TalaiotExtension(project) {
*/
var publishers: InfluxdbConfiguration? = null

fun publishers(block: PublishersConfiguration.() -> Unit) {
fun publishers(block: InfluxdbConfiguration.() -> Unit) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this is the fix for "InfluxDbPublisherConfiguration cannot be found" error, right?

Copy link
Owner Author

Choose a reason for hiding this comment

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

exactly. Sorry about that, the test is in groovy and at the company we are using the standard version.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems the same problem I've described at #245 (comment). 🤔

Copy link
Owner Author

@cdsap cdsap Nov 24, 2020

Choose a reason for hiding this comment

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

hi @mokkun, my apologies, I went again to the comment and you were completely right. Checking my response I was testing plugins { id("com.cdsap.talaiot") version "1.4.0" } without seeing the problem you pointed https://github.com/cdsap/Talaiot/pull/258/files#diff-05c8c3de206dc8f64274e3a9bb02c7696eb8ebbd523e30f3be4972bb235cac15L16

Copy link
Collaborator

@mokkun mokkun left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@MyDogTom
Copy link
Collaborator

@cdsap Unfortunately, I still have an issue updating to the version 1.4. Perhaps I'm missing something obvious.
I created an example project: https://github.com/MyDogTom/talaiot-dependencies-issue . The project was generated by gradle init, that's why there are some noise. Actual code is located in ProjectFolderMetric and TalaiotCustomPluginPlugin.

This is how I declare dependencies

    implementation("com.cdsap:talaiot:1.4.0")
    implementation("com.cdsap.talaiot.plugin:base:1.4.0")
    implementation("com.cdsap.talaiot.plugin:influxdb:1.4.0")

I added com.cdsap:talaiot:1.4.0 as a last resort, but it didn't help. I guess that ideally I'd need only base and influxdb dependencies, right?

I created a ProjectFolderMetric custom metric

It's broken because it cannot resolve com.cdsap.talaiot.metrics.base.GradleMetric.

I also create a custom plugin that applies Talaiot plugin

It's broken because

Cannot access 'com.cdsap.talaiot.TalaiotExtension' which is a supertype of 'com.cdsap.talaiot.plugin.influxdb.InfluxdbExtension'. Check your module classpath for missing or conflicting dependencies

It seems that I miss something obvious, but I cannot find out on my own what exactly :(

@cdsap
Copy link
Owner Author

cdsap commented Nov 24, 2020

Hi @MyDogTom
Thanks for the test repository finally I was able to understand better the problem.

The idea of your test project is implement a new plugin including specific configuration for metrics, filters and specific configuration for the influxdb publisher.
It's totally valid the use case. But I want to remark first the implications of decomposing the different components of the library:
-- core
-- publishers
-- plugins

Plugins are just the entry points to specific publisher/s implementing the core library. They extend the logic of the new "talaiot" configuration with the details of the publisher, as well they create the Talaiot Plugin of the core library.
The plugins are intended to work integrated in the build class path of the gradle project.

When we separated the publishers we opened the possibility to create new plugins using the existing publishers. For example, I want to create a plugin using influxdb-publisher to add some extra configuration.

Taking your project the implementation would be:
1- Dependencies

    implementation("com.cdsap.talaiot:talaiot:1.4.0")
    implementation("com.cdsap.talaiot:influxdb-publisher:1.4.0")

Why Talaiot? Because it has the core functionality and we are creating a new Plugin, and why InfluxDb? Because is the publisher we are extending and we can reuse the conf given by the influxdb-publisher.

2- Create the Metrics, now import com.cdsap.talaiot.metrics.base.GradleMetric is accessible because belongs to the core.

3- Create the plugin following the same convention used in the new ones:

    override fun apply(project: Project) {
        Talaiot(
                InfluxdbExtension::class.java,
                InfluxdbConfigurationProvider(
                        project
                )
        ).setUpPlugin(project)

       

4- And then setting up your default plugin:

 val talaiotExtension = project.extensions.getByName("talaiot") as InfluxdbExtension
        talaiotExtension.metrics {
            gitMetrics = false
            customMetrics(
                    ProjectFolderMetric()
            )

            talaiotExtension.publishers {
                influxDbPublisher {
                    dbName = "test-database"
                    url = "to-do"
                    taskMetricName = "task"
                    buildMetricName = "build"
                    publishTaskMetrics = true
                }
            }

            talaiotExtension.filter {
                build {
                    success = true
                    requestedTasks {
                        excludes = arrayOf("myTask")
                    }
                }
            }

            talaiotExtension.ignoreWhen {
                envName = "RUN_ON_CI"
                envValue = "true"
            }

And that's how I interpret the test repository, creating a new Plugin to be consumed by external dependencies with a default configuration for metrics and influxdb.
If you are using Talaiot in the build classpath with:

plugins {
    id("com.cdsap.talaiot") version "1.4.0"
}

you have access to all dependencies included in the plugin applied(core/publishers) and you can extend the funcionality with the DSL.

I want to ask you again about the opinion because I understand that adding api configurations will fix the problem, but in terms of design I think it's more clear for the plugins authors to extend from publishers and core library. Even in your use case I think creating a TalaiotPlugin with a default conf will be more clear.

On the downsides, I see the complexity of the creating of a Plugin:
-- Creating extension
-- Configuration
-- ConfigurationProvider
Of course it should be improved.

Ah, I see you are including base, I think the name is not clear, the base plugin contains base publishers like Json or TimeLine, the name is not very good.

@MyDogTom
Copy link
Collaborator

Thank you @cdsap ! I applied your instructions to the example project and it compiles. The only difference is that I had to add the dependency to the influxdb-publisher

    implementation("com.cdsap.talaiot:talaiot:1.4.0")
    implementation("com.cdsap.talaiot.plugin:influxdb:1.4.0")
    implementation("com.cdsap.talaiot:influxdb-publisher:1.4.0")

without com.cdsap.talaiot:influxdb-publisher:1.4.0 compilation fails with

e: /.../TalaiotCustomPluginPlugin.kt: (45, 17): Unresolved reference: dbName
e: /.../TalaiotCustomPluginPlugin.kt: (46, 17): Unresolved reference: url
e: /.../TalaiotCustomPluginPlugin.kt: (47, 17): Unresolved reference: taskMetricName
e: /.../TalaiotCustomPluginPlugin.kt: (48, 17): Unresolved reference: buildMetricName
e: /.../TalaiotCustomPluginPlugin.kt: (49, 17): Unresolved reference: publishTaskMetrics

@MyDogTom
Copy link
Collaborator

I also tried it out in my main project and so far it looks good!

@cdsap
Copy link
Owner Author

cdsap commented Dec 2, 2020

Hi, sorry I was moving to new apartments last days, I will check these days everything

@cdsap cdsap merged commit b136bdc into master Dec 2, 2020
@cdsap cdsap mentioned this pull request Dec 4, 2020
@MyDogTom MyDogTom mentioned this pull request Dec 10, 2020
@cdsap cdsap deleted the fix_and_prepare_release_1.4 branch April 5, 2021 02:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants