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

feat: extract flows and templates #984

Merged
merged 9 commits into from
Feb 21, 2023
Merged

Conversation

loicmathieu
Copy link
Member

@loicmathieu loicmathieu commented Feb 13, 2023

Allow extracting Flows and Templates

@Skraye
Copy link
Member

Skraye commented Feb 13, 2023

You may follow the same pattern as bulk actions on executions to keep consistency and easier implementation in the front

@tchiotludo
Copy link
Member

Totally agree, /extract/by-ids and /extract/by-query is better I think

@loicmathieu loicmathieu force-pushed the extract-flows-and-templates branch 7 times, most recently from cbf4d51 to acfeaed Compare February 15, 2023 08:31
@loicmathieu loicmathieu marked this pull request as ready for review February 15, 2023 09:17

try(DefaultHttpClient client = client()) {
MutableHttpRequest<Object> request = HttpRequest
.GET("/api/v1/flows/extract/by_query?namespace=" + namespace).accept(MediaType.APPLICATION_OCTET_STREAM);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.GET("/api/v1/flows/extract/by_query?namespace=" + namespace).accept(MediaType.APPLICATION_OCTET_STREAM);
.GET("/api/v1/flows/extract/query?namespace=" + namespace).accept(MediaType.APPLICATION_OCTET_STREAM);

Copy link
Member Author

@loicmathieu loicmathieu Feb 16, 2023

Choose a reason for hiding this comment

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

Well, see your comment bellow

Totally agree, /extract/by-ids and /extract/by-query is better I think

I'm OK to change it but as extract is a verb by-query reads better than just query.

Copy link
Member Author

Choose a reason for hiding this comment

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

Moreover, if we keep by-ids we should use by-query

@@ -25,6 +31,16 @@
@ToString
@EqualsAndHashCode
public class Template implements DeletedInterface {

private static final ObjectMapper jsonMapper = JacksonMapper.ofJson().copy()
Copy link
Member

Choose a reason for hiding this comment

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

Too many duplicate from flow

@@ -93,6 +109,23 @@ public Optional<ConstraintViolationException> validateUpdate(Template updated) {
}
}

public String generateSource() {
Copy link
Member

Choose a reason for hiding this comment

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

this method is not necessary, a simple mapper will generate the source on templates

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I setup the object mapper once for all and use it directly.
By the way, I think the same can be done on the flow class, as the Flow.generateSource() method is now only used in tests this should be fine to change it the same way I did for templates.

@@ -416,4 +421,59 @@ public List<ValidateConstraintViolation> validateFlows(
})
.collect(Collectors.toList());
}

@ExecuteOn(TaskExecutors.IO)
@Get(uri = "/extract/by_query", produces = MediaType.APPLICATION_OCTET_STREAM)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@Get(uri = "/extract/by_query", produces = MediaType.APPLICATION_OCTET_STREAM)
@Get(uri = "/extract/query", produces = MediaType.APPLICATION_OCTET_STREAM)

ZipOutputStream archive = new ZipOutputStream(bos)) {

List<FlowWithSource> toExtract = flowRepository.findWithSource(query, namespace, RequestUtils.toMap(labels));
for(var flowWithSource : toExtract) {
Copy link
Member

Choose a reason for hiding this comment

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

too many duplication, create a service that create the zip

Copy link
Member Author

Choose a reason for hiding this comment

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

We cannot share the zip code between template and flow easily so I create private methods in each class to avoid duplication (this was my originla plan but I forgot to do it).

@loicmathieu loicmathieu force-pushed the extract-flows-and-templates branch 2 times, most recently from 1f6706b to 924f391 Compare February 17, 2023 09:03
@tchiotludo tchiotludo mentioned this pull request Feb 20, 2023
@loicmathieu loicmathieu force-pushed the extract-flows-and-templates branch 3 times, most recently from e1a4b67 to 4ef4aba Compare February 20, 2023 14:05
@tchiotludo tchiotludo force-pushed the extract-flows-and-templates branch 3 times, most recently from cdbd192 to 9d9c672 Compare February 20, 2023 22:11
@tchiotludo tchiotludo force-pushed the extract-flows-and-templates branch from f840eed to b690831 Compare February 21, 2023 07:54
@tchiotludo tchiotludo merged commit a227ff3 into develop Feb 21, 2023
@tchiotludo tchiotludo deleted the extract-flows-and-templates branch February 21, 2023 13:35
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