Conversation
mnonnenmacher
force-pushed
the
resolution-sources
branch
2 times, most recently
from
abfcefc to
693681f
Compare
mnonnenmacher
requested review from
MarcelBochtler,
bs-ondem,
nnobelis,
oheger-bosch,
sschuberth and
wkl3nk
as code owners
workers/common/src/main/kotlin/resolutions/OrtServerResolutionProvider.kt
Outdated
Show resolved
Hide resolved
workers/common/src/main/kotlin/resolutions/OrtServerResolutionProvider.kt
Show resolved
Hide resolved
workers/common/src/test/kotlin/resolutions/OrtServerResolutionProviderTest.kt
Show resolved
Hide resolved
| REPOSITORY, | ||
|
|
||
| /** The resolution is managed by the server. */ | ||
| SERVER |
There was a problem hiding this comment.
I assume, this refers to resolutions that have been created via the UI, right? The name SERVER does not seem to fit, also the comment "...managed by the server". I have, however, no better idea either.
Also, REPOSITORY might be a bit misleading, since it refers only to the configuration file for this repository and not the structure in the hierarchy.
There was a problem hiding this comment.
Maybe we can clarify all of these enum names a bit, like: GLOBAL_FILE, REPOSITORY_FILE, SERVER_UI. Instead of the "_FILE" suffix maybe also "_CONFIG" could be used.
And BTW, I do not think that a "_UI" suffix would be misleading here, just because resolutions could also be sent via curl to the API directly, because this should more indicate how that type of resolution is supposed to be managed, so I believe saying "it's supposed to be managed via the UI" is fine.
There was a problem hiding this comment.
I don't want to do the refactoring before we agree on the names, so what about these:
GLOBAL_FILEREPOSITORY_FILESERVER_REPOSITORY: This is precise because these resolutions are associated to a specific repository. It is also future proof in case we ever want to create resolutions also on a different hierarchy level (which could make sense for issue resolutions).
There was a problem hiding this comment.
I'm not sure about SERVER_REPOSITORY in the context of #2938; I'd rather not refer to a "repository" here. It's also misleading as it sounds as if the resolution would still somehow be stored in the Git repository along with the source code. If at all, maybe say SERVER_REPOSITORY_LEVEL.
There was a problem hiding this comment.
I agree, _REPOSITORY would be misleading, but let's not invent any monster names, either... 😏
Why can't we just use SERVER?
There was a problem hiding this comment.
Still not really happy with SERVER, but as I have no better idea, I do not object.
There was a problem hiding this comment.
@sschuberth @Etsija None of you has made a change request, so do you insist on changing the names or not?
There was a problem hiding this comment.
I have no objections to the original ones, but if @sschuberth wants to, I'm also OK with GLOBAL_FILE, REPOSITORY_FILE, and SERVER.
There was a problem hiding this comment.
I somewhat do insist on having the "_FILE" suffixes at least. Let's briefly discuss in the daily.
There was a problem hiding this comment.
We decided to go with GLOBAL_FILE, REPOSITORY_FILE, and SERVER (for now).
mnonnenmacher
force-pushed
the
resolution-sources
branch
from
693681f to
17473eb
Compare
mnonnenmacher
force-pushed
the
resolution-sources
branch
from
17473eb to
ccb9049
Compare
|
From my PoV, the changes are fine. |
Create an `OrtServerResolutionProvider` to prepare for storing the source of resolutions. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@doubleopen.org>
Change all workers to use the new provider and remove the now unused helper functions. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@doubleopen.org>
Extend the three resolution types with a new `source` property which stores where the resolution is coming from. This commit is applying the required model and database schema changes, the following commits will implement storing the correct source for each resolution. For resolutions already stored in the database, the source `REPOSITORY_FILE` is chosen which represents the repository configuration file, as this is most likely correct for the majority of past resolutions. This avoids introducing an `UNKNOWN` source only for historic reasons which will never be used in the future. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@doubleopen.org>
Move the `resolveResolutionsWithMappings` helper function to the `OrtServerResolutionProvider` to prepare for setting the correct sources for resolutions. Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@doubleopen.org>
Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@doubleopen.org>
mnonnenmacher
force-pushed
the
resolution-sources
branch
from
ccb9049 to
c8e96ba
Compare
|
Looks good from my part. UI Docker build fails, however. |
Yes, there are issues with repo.eclipse.org currently: |
4 participants
Store from which source resolutions are originating (global configuration file, repository configuration file, or the server). This is mainly required for the UI later on to decide which resolutions are editable. See the commit messages for details.