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

Update project strcuture publish #29

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

thisAAY
Copy link
Contributor

@thisAAY thisAAY commented Oct 11, 2024

  • Updated the publishing setup
  • Updated Gradle from 4.9 to 8.9
  • Updated JDK from 7 to 17
  • Updated kotlin from 1.3.72 to 2.0.20
  • Updated Deps
  • Migrated to Version Catalog
  • Refactored to multi-module setup to include the library and samples, Instead of having two different projects

Copy link

sonarcloud bot commented Oct 11, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
2 Security Hotspots
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

Copy link
Collaborator

@KCeh KCeh left a comment

Choose a reason for hiding this comment

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

Kudos for big upgrades 👏
Let's make sure at least bitrise passes. As for sonar error I would put them into "would be nice to fix but not blocker" category. Since they are not new, Sonar scan on repo is new

Comment on lines +256 to +257
publish.properties
*.gpg
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Comment on lines +52 to +65
developers {
developers {
developer {
id.set("antunflas")
name.set("Antun Flaš")
email.set("[email protected]")
}
}
developer {
id = 'thisAAY'
name = 'Ahmed Ali'
email = '[email protected]'
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not really a blocker or big thing. But we should replace personal info with company's info.

organization {
    name = 'Infinum Inc.'
    url = 'https://infinum.com'
}
developers {
        developer {
        id = 'Infinum'
        name = 'Infinum Inc.'
        url = 'https://infinum.com'
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we do this for all libs? because i think this is the same setup for JsonApiX too

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, but it is not big priority, you can fix it on JsonApiX when you will do some work there

Comment on lines 30 to 35
version = "$versions.retromock"

artifact sourceJar
artifact javadocJar

pom {
name.set("Retromock")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't we just apply project level publish file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just for feature reasons, in case we have different modules that can be published, so I just went with our current setup

Copy link
Member

Choose a reason for hiding this comment

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

This file should not be pushed to remote

Copy link
Member

Choose a reason for hiding this comment

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

Same here, just remove it from the project, but history can stay as is 🤷


username: findProperty("sonatypeUsername")?.toString()
?: properties["sonatype.user"]
?: System.getenv("SONATYPE_USERNAME"),
Copy link
Member

Choose a reason for hiding this comment

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

Btw, I have these env variables defined and I avoid using publish.properties, because it works on all projects automatically, and don't have to think about updating gitignore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I would suggest removing this line from every library and the template if it exists there, so we don't encourage using a file that can be easily pushed to gitignore

Copy link
Member

Choose a reason for hiding this comment

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

cc @KCeh, let's plan to remove publish.properties from the library projects then.

Copy link
Collaborator

Choose a reason for hiding this comment

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

👌
I will recheck all libs, in library template there is no such file

Copy link
Member

Choose a reason for hiding this comment

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

I'm sorry, I didn't mean to check if there is a file. I suggested removing the usage of the properties file in this file.

Copy link
Member

Choose a reason for hiding this comment

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

This can also be in gitignore, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will update gitinore again, and force some updates in the history of this branch

Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be harmful, so we can leave it in history.

@@ -30,7 +30,6 @@ afterEvaluate {
version = "$versions.retromock"

artifact sourceJar
artifact javadocJar
Copy link
Member

Choose a reason for hiding this comment

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

Is the doc automatically added?

Copy link
Member

@antunflas antunflas left a comment

Choose a reason for hiding this comment

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

Looks good! I'll re-review after you address the comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants