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 configuration #27

Merged
merged 4 commits into from
Jan 16, 2023
Merged

Add support for configuration #27

merged 4 commits into from
Jan 16, 2023

Conversation

azdrojowa123
Copy link
Collaborator

No description provided.

@azdrojowa123 azdrojowa123 linked an issue Jan 12, 2023 that may be closed by this pull request
@azdrojowa123 azdrojowa123 self-assigned this Jan 12, 2023
@azdrojowa123 azdrojowa123 changed the title Add configuration file Add support for configuration Jan 12, 2023
@azdrojowa123 azdrojowa123 force-pushed the configuration-support branch 2 times, most recently from 95eccb3 to da18eef Compare January 13, 2023 11:46
@@ -0,0 +1,16 @@
maven:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am guessing that this is some example file?
Shouldn't we just put it in some test resources and validate if it is used correctly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I forgot about the tests so I will create them

@@ -1,5 +1,6 @@
package org.virtuslab.bazelsteward.app

import config.src.main.kotlin.org.virtuslab.bazelsteward.config.BazelStewardConfiguration
Copy link
Collaborator

Choose a reason for hiding this comment

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

something bad happened in terms of imports


private val configFilePath = repoRoot.resolve(".bazel-steward.conf")

suspend fun get(): Configuration? {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is nullable. Is there any point for getting null from here?
If there is no file, return empty configuration and if there are errors, throw.

import java.nio.file.Path

data class Configuration(
val maven: MavenConfig?
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd avoid nullability in most places if there is a natural way to have something empty, i.e. just have

val maven: MavenConfig = MavenConfig()
  val ruledDependencies: List<MavenDependency> = emptyList()

It will be easier to use it in code later

runCatching {
Files.newBufferedReader(configFilePath).use { bufferedReader ->
val configContent =
bufferedReader.use { br -> br.readLines().filterNot { it.startsWith("#") }.joinToString("\n") }
Copy link
Collaborator

Choose a reason for hiding this comment

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

doesn't yaml support comments natively?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

unfortunately not, commented lines are loaded


class BazelStewardConfiguration(repoRoot: Path) {

private val configFilePath = repoRoot.resolve(".bazel-steward.conf")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that later we will have a number of places where we look for it, we might also have various extensions, but now maybe let's look for .bazel-steward.yaml for clarity? .conf may be associated with different formats.

@azdrojowa123 azdrojowa123 force-pushed the configuration-support branch 6 times, most recently from b01b3c5 to 5d30768 Compare January 16, 2023 09:57
suspend fun get(): Configuration {

return withContext(Dispatchers.IO) {
val schemaContent = javaClass.classLoader.getResource("bazel-steward-schema.json")?.readText() ?: return@withContext Configuration()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is rather a serious error in our deployment if we cannot access our schema to validate the config. I'd rather throw here.

@lukaszwawrzyk lukaszwawrzyk enabled auto-merge (squash) January 16, 2023 12:53
@lukaszwawrzyk lukaszwawrzyk merged commit c83cea0 into main Jan 16, 2023
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.

Add support for configuration
2 participants