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/koto xml import #707

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Feat/koto xml import #707

wants to merge 13 commits into from

Conversation

saku-koodari
Copy link
Contributor

@saku-koodari saku-koodari commented Jan 21, 2025

Tehtäväpankkien tuonti

  • Oon tehnyt sille koto-alustan alle oman domainin, jotta ei sekottuis muiden koto asioiden kanssa
  • JsonMapperiin ja WebClienttiin piti nostaa rajoitus 128MB, jotta ei tule BufferLimitException - tyyppisiä virheitä
  • Jotta saan metatietoja tiedoston nimeen, teen serialisoinnin stringiin. Tästä tulee pientä performanssi hittiä. Voiskohan sitä optimoida?
  • Pihvi on tehtavapankki/TehtavapankkiService.kt/importTehtavapankki
    • haetaan Mono:na (kotlin coroutineilla), jotta saadaan niin iso response sisään.
    • serialisoidaan se TehtavapankkiResponseen
    • iteroidaan jokainen tehtäväpankin objekti läpi, generoidaan niillä {KURSSIN_ID}-{KURSSIN_NIMI}/{ISO_DATETIME_NOW}-{INDEX}.xml - niminen tiedosto.
    • Yritetään tallentaa se S3:een.

Kesken:

  • Tällä hetkellä tiedoston nimi saattaa olla liian pitkä. Mulla tuli jotain virheitä kehityksen aikana, ja veikkaan et ne johtu siitä.
  • Tunnarit ei oo kunnossa. Tällä hetkellä tulee: The AWS Access Key Id you provided does not exist in our records. (Service: S3, Status Code: 403,

@Kailari Kailari force-pushed the feat/koto-xml-import branch 2 times, most recently from 0052c8e to fcc2102 Compare January 31, 2025 12:57
@Kailari Kailari force-pushed the feat/koto-xml-import branch 2 times, most recently from db8adcc to 7fd6dc0 Compare February 11, 2025 12:52
@Kailari Kailari force-pushed the feat/koto-xml-import branch from 7fd6dc0 to 9a0cf5b Compare February 11, 2025 12:57
@Kailari Kailari force-pushed the feat/koto-xml-import branch from 38084c6 to a8324f2 Compare February 11, 2025 13:07
@Kailari Kailari force-pushed the feat/koto-xml-import branch from a8324f2 to 12f0e4d Compare February 11, 2025 13:14
@Kailari Kailari force-pushed the feat/koto-xml-import branch from f5a5dcd to 27147ec Compare February 12, 2025 12:40
@Kailari Kailari force-pushed the feat/koto-xml-import branch from 27147ec to f5568c7 Compare February 12, 2025 13:48
@JsonProperty("questionbanks")
val questionbanks: List<Questionbank>,
) {
data class Questionbank(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
data class Questionbank(
data class QuestionBank(

Kirjoittaisin tuon englanniksi tekstissä kahtena sanana "question bank", joten pitäisi koodissakin. Toisaalta pitäisikö tämä olla suomeksi vaan koodissakin, eli Kysymyspankki?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mä ajattelin pitää siinä muodossa kuin missä se lähdejärjestelmässä on, välittämättä oikeasta muodosta.

lateinit var tehtavapankkiImportSchedule: String

@Bean
fun dailyImportKotoTehtavapankki(tehtavapankkiService: TehtavapankkiService): Task<Void> =
Copy link
Contributor

Choose a reason for hiding this comment

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

Voisiko tämän laittaa KoealustaScheduledTasks-luokkaan? Eli jos yhdessä luokassa voi olla useampi Task-tyyppinen metodi, niin ei tarvisi erillistä luokkaa tälle.

Copy link
Contributor Author

@saku-koodari saku-koodari Feb 20, 2025

Choose a reason for hiding this comment

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

Tää on nyt tehty 3D - formaatissa ja niin, että tehtäväpankki olisi yksi kotoutumiskoulutus:sen alidomain. Jos ei haluta tällaista, niin mun mielestä on sit turha pitää koko alidomainia, ja se voidaan purkaa, flatata kotoutumiskoulutus:kseen.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alidomain on fine, tämä on hyvä selitys!

Copy link
Contributor

Choose a reason for hiding this comment

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

voiks DDD:n lyhentää 3D?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

voiks DDD:n lyhentää 3D?

Korjattu

@@ -1,71 +0,0 @@
package fi.oph.kitu.logging
Copy link
Contributor

Choose a reason for hiding this comment

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

Onko OK tämän poisto, vai pitäisikö testi muuttaa uutta tapaa testaavaksi?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noi testit näyttää testaavan vain withEvent:ia. Nythän me ollaan siirretty tuon vastuu OTEL:lle, niin se testaaminen kuulunee OTEL:lle

saku-koodari and others added 12 commits February 20, 2025 16:35
in order to distinct from tehtavapankki url
The `withEvent` seems to be unused now. All of its usages have been
replaced with `withEventAndPerformanceCheck`, so using that should be
preferred instead.
How often the import should run etc. is still bit of a open question,
so just disable the import to remove blockers for merging this.
Allow the bucket property to be undefined (default to null) and treat a
missing bucket name as a dry-run.

This reduces chances of an accidental S3 upload from local env, while
cicumventing the need to configure a bucket name for the local env, too.
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.

4 participants