Skip to content
This repository has been archived by the owner on Jan 20, 2023. It is now read-only.

Naming convention #49

Merged
merged 5 commits into from
Apr 24, 2019
Merged

Naming convention #49

merged 5 commits into from
Apr 24, 2019

Conversation

garagatyi
Copy link

What does this PR do?

Make broker download metas using new naming convention.
Details at eclipse-che/che-plugin-registry#55
Cleanup not needed code.

What issues does this PR fix or reference?

Related to eclipse-che/che-plugin-registry#55

Oleksandr Garagatyi added 2 commits April 19, 2019 14:00
Details at eclipse-che/che-plugin-registry#55
Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>
Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>
@codecov
Copy link

codecov bot commented Apr 19, 2019

Codecov Report

Merging #49 into master will increase coverage by 0.42%.
The diff coverage is 82.35%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #49      +/-   ##
==========================================
+ Coverage   58.35%   58.77%   +0.42%     
==========================================
  Files           8        8              
  Lines         802      803       +1     
==========================================
+ Hits          468      472       +4     
+ Misses        289      286       -3     
  Partials       45       45
Impacted Files Coverage Δ
storage/storage.go 88.88% <100%> (+0.65%) ⬆️
brokers/theia/broker.go 63.72% <100%> (ø) ⬆️
brokers/che-plugin-broker/broker.go 65.69% <100%> (ø) ⬆️
brokers/unified/broker.go 53.78% <57.14%> (+2.11%) ⬆️
brokers/vscode/broker.go 71.49% <85.71%> (ø) ⬆️

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 e93ad4d...675afc0. Read the comment docs.

Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

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

LGTM

cfg/cfg.go Show resolved Hide resolved
Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

LGTM

cfg/cfg.go Outdated
"download-metas",
false,
"Download plugin metadata from registry instead of process already-downloaded metas",
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing this now would result in a minor version bump (0.16.x) and require changes in upstream Che.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, but I'm considering change version to v1.0.0 since it is backward incompatible and first number is supposed be reflect such changes. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm open to either option but worry we're not to the point of calling it a v1.0 release, especially with the various naming convention/registry changes still coming down the pipe. It's still okay to break backward compatibility in version zero so I'm leaning more in that direction.

cfg/cfg.go Outdated Show resolved Hide resolved
@amisevsk
Copy link
Contributor

Could you also remove --download-metas from the Makefile?

@AndrienkoAleksandr AndrienkoAleksandr changed the title Namng convention Naming convention Apr 24, 2019
@garagatyi garagatyi merged commit c90e7d4 into master Apr 24, 2019
@garagatyi garagatyi deleted the namngConvention branch April 24, 2019 13:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants